MRChemSoft / mrchem

MultiResolution Chemistry
GNU Lesser General Public License v3.0
27 stars 21 forks source link

Bank upgrade #362

Closed gitpeterwind closed 3 years ago

gitpeterwind commented 3 years ago

Refactoring of Bank. Main changes:

codecov[bot] commented 3 years ago

Codecov Report

Merging #362 (83500d5) into master (f51902e) will increase coverage by 0.07%. The diff coverage is 29.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
+ Coverage   69.08%   69.16%   +0.07%     
==========================================
  Files         171      171              
  Lines       13290    13298       +8     
==========================================
+ Hits         9181     9197      +16     
+ Misses       4109     4101       -8     
Impacted Files Coverage Δ
src/driver.cpp 70.31% <ø> (-0.05%) :arrow_down:
src/parallel.cpp 82.60% <ø> (+33.79%) :arrow_up:
src/qmfunctions/OrbitalIterator.cpp 43.08% <0.00%> (ø)
src/qmfunctions/OrbitalIterator.h 100.00% <ø> (ø)
src/qmoperators/two_electron/ExchangePotential.h 40.00% <0.00%> (-10.00%) :arrow_down:
src/qmoperators/two_electron/ExchangePotentialD1.h 100.00% <ø> (ø)
src/qmoperators/two_electron/ExchangePotentialD2.h 100.00% <ø> (ø)
src/scf_solver/GroundStateSolver.cpp 89.68% <ø> (-0.05%) :arrow_down:
src/scf_solver/SCFSolver.cpp 95.83% <0.00%> (ø)
src/qmfunctions/orbital_utils.cpp 67.64% <20.83%> (+0.26%) :arrow_up:
... and 5 more

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 f51902e...83500d5. Read the comment docs.

stigrj commented 3 years ago

This looks like a good step in the right direction. One general problem we have is that the MPI parts are not very well tested, so we should get a few unit tests up and running here. I also had to disable MPI for the integration tests some time ago since they refused to launch correctly after runtest was removed. I will try to set up a testing framework for the banking, then I might come back to @gitpeterwind for help with making actual tests.

gitpeterwind commented 3 years ago

Yes, a lot of tests including MPI would be useful. It would however not be a big disadvantage to run those outside github.

gitpeterwind commented 3 years ago

I plan also to make new PR relatively soon, which also include some changes on the Bank (I also put the "enum" requested by @robertodr ). So if there are changes to be made in this PR, it would be better to take it into the new directly.