Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Odeint with in-house Runge-Kutta 4th order integrator #138

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

robertodr
Copy link
Member

@robertodr robertodr commented Mar 22, 2018

Description

This PR removes the dependency on Odeint by providing code for a Runge-Kutta 4th order integrator.
In addition, the code for handling the radial solutions and some of the internals of the SphericalDiffuse class have been significantly simplified.

How Has This Been Tested?

All unit tests but one pass. green_spherical_diffuse, which checks the value of the Green's function. However, the tests on boundary integral operators and IEF pass without any tweak. It seems to me some loss of accuracy happens with the new integrator, but I cannot see how that could happen given that the Butcher tableau used by Odeint is the same as mine.

Todos

  • Developer Interest
  • Dropped dependency on Boost.Odeint

Questions

  • @ilfreddy please have a look and make sure it's fine to update the reference values for the Green's function tests.

Status

  • Ready to go

@robertodr robertodr requested a review from ilfreddy March 22, 2018 22:34
@MinazoBot
Copy link

MinazoBot commented Mar 22, 2018

1 Warning
⚠️ Consider adding supporting documentation to this change. Documentation sources can be found in the doc directory.

Generated by 🚫 Danger

@ilfreddy
Copy link
Collaborator

ilfreddy commented Mar 23, 2018

I don't see that you have included the modifications to the reference values in the test file. If you want me to evaluate whether the difference is significant or not, you should update them. Or you want me to run the tests myself and see?

@robertodr
Copy link
Member Author

The latter. It's probably a good idea if you check this out on stallo and re-run the scans you used to test for the L=0 bug.

@robertodr robertodr changed the title [WIP] Replace Odeint with in-house Runge-Kutta 4th order integrator Replace Odeint with in-house Runge-Kutta 4th order integrator Mar 27, 2018
@ilfreddy
Copy link
Collaborator

I have checked the results against my test set of interfaces. The results are practically identical. Unfortunately the old results were printed with six digits accuracy only and I can therefore not go beyond that, but all numbers match to the last digit available (just some rounding differences).
This is therefore good to go as soon as the reference values are updated.

@ilfreddy
Copy link
Collaborator

Maybe we should keep the interface reference calculations (inp/out) somewhere on github, just in case? They are too long to be used as routine tests, but they might turn useful every now and then.

@robertodr
Copy link
Member Author

We had a pcmsolvermeta repository for this kind of stuff when we were on GitLab. I didn't port that repo, but you're welcome to do that.

@codecov
Copy link

codecov bot commented Mar 27, 2018

Codecov Report

Merging #138 into master will increase coverage by 0.24%.
The diff coverage is 89.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   69.83%   70.07%   +0.24%     
==========================================
  Files          92       93       +1     
  Lines        5616     5601      -15     
==========================================
+ Hits         3922     3925       +3     
+ Misses       1694     1676      -18
Impacted Files Coverage Δ
src/utils/MathUtils.hpp 96.96% <ø> (-1.06%) ⬇️
src/interface/Meddle.cpp 71.13% <ø> (ø) ⬆️
src/green/SphericalDiffuse.hpp 44.44% <ø> (ø) ⬆️
src/utils/Molecule.hpp 100% <100%> (ø) ⬆️
src/utils/Symmetry.cpp 100% <100%> (ø) ⬆️
src/utils/RungeKutta.hpp 100% <100%> (ø)
src/utils/Molecule.cpp 74.46% <66.66%> (ø) ⬆️
src/green/SphericalDiffuse.cpp 80% <69.23%> (-1.46%) ⬇️
src/utils/Symmetry.hpp 76.19% <73.68%> (-23.81%) ⬇️
src/green/InterfacesImpl.hpp 77.27% <83.72%> (-2.34%) ⬇️
... and 4 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 5133636...8b2f3a9. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants