robashaw / libecpint

A C++ library for the efficient evaluation of integrals over effective core potentials.
MIT License
28 stars 15 forks source link

replaced C-style arrays with std::vector #26

Closed JensWehner closed 3 years ago

JensWehner commented 3 years ago

I got mismatched free/delete errors in bessel.cpp

as <vector> is included anyway I just replaced the double*** with nested std::vectors, performance wise it should not matter otherwise I can also replace them with the 2-index,3-index classes.

Also fixed the copy constructor of ECP object.

codecov[bot] commented 3 years ago

Codecov Report

Merging #26 (318b244) into master (5731482) will decrease coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   89.73%   89.71%   -0.03%     
==========================================
  Files          19       19              
  Lines        1958     1954       -4     
==========================================
- Hits         1757     1753       -4     
  Misses        201      201              
Impacted Files Coverage Δ
src/lib/bessel.cpp 97.84% <100.00%> (-0.11%) :arrow_down:
src/lib/ecp.cpp 97.64% <100.00%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5731482...318b244. Read the comment docs.

junghans commented 3 years ago

@robashaw I would need this, too!

robashaw commented 3 years ago

Thanks for this. I need to run some benchmarking tests because even a tiny change in the efficiency of the Bessel function routines can massively slow down everything else. I'll try to do that soon

JensWehner commented 3 years ago

Sure, if we you want to make the tables fast then using https://github.com/robashaw/libecpint/blob/master/include/libecpint/multiarr.hpp should be much better, because less pointer indirections.

JensWehner commented 3 years ago

@robashaw any verdict? Thx

nabbelbabbel commented 3 years ago

Updating the current destructor also works. If the decision to switch to other containers is to involved:

    BesselFunction::~BesselFunction() {
                for (int i = 0; i < N+1; i++) {
                        for (int j = 0; j < TAYLOR_CUT + 1; j++)
                                delete[] dK[i][j];
                        delete[] K[i];
                        delete[] dK[i];
                }
        delete[] K;
        delete[] dK;
        delete[] C;
    }
junghans commented 3 years ago

@robashaw any news on this?

robashaw commented 3 years ago

I’m sorry that I’ve not responded yet, I’ve been ill, but I’ll try to have a look at it tonight.

On 24 Jan 2021, at 20:31, Christoph Junghans notifications@github.com wrote:

 @robashaw any news on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

robashaw commented 3 years ago

I have benchmarked several build cases and can see no discernible difference, as the compiler is essentially translating the vector-based exactly the same way as the C-array-based code. It is plausible this will not be the case for older compiler versions, but I'm making an executive decision to neither worry nor care about that.

Thanks for the request and sorry for the delay.

junghans commented 3 years ago

Could make a new patch release as well?

junghans commented 3 years ago

@robashaw could you make another 1.0 release, I will like to package this fix for fedora.