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

Ias15 implentation #643

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Ias15 implentation #643

wants to merge 7 commits into from

Conversation

johnwez1
Copy link

@johnwez1 johnwez1 commented May 12, 2024

Implemented the ias15 algorithm in c with python wrappers, and added to relevant tests.
Some notes:

  • Didnt implement the “begins to oscillate”
  • Used "global" calculations.
  • Didn't implement user warning for 12 iterations
  • My implementation treats each step as an independant integration, with dynamic stepping in between. This part seems a bit silly but I thought at least I would give a working version at first. Otherwise, I would need to create an interpolation function to match the other integrators.
  • Slight concern that it might get caught in infinite loop, in the while time remaining > 0 loop.

Copy link

codecov bot commented May 12, 2024

Codecov Report

Attention: Patch coverage is 97.43590% with 6 lines in your changes missing coverage. Please review.

Project coverage is 99.89%. Comparing base (919aae8) to head (f2f9a23).
Report is 33 commits behind head on main.

Files Patch % Lines
galpy/util/wez_ias15.c 97.19% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   99.01%   99.89%   +0.87%     
==========================================
  Files         200      202       +2     
  Lines       29260    29494     +234     
  Branches      563      604      +41     
==========================================
+ Hits        28973    29464     +491     
+ Misses        287       30     -257     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jobovy
Copy link
Owner

jobovy commented May 15, 2024

Thanks for this PR! I'll need a bit of time to work through this and familiarize myself better with IAS15 to be able to review this. But in the meantime, it would be good to fix some of the failing tests. It seems like some of the orbit integration tests fail for the new integrator (these are run on different OSs and for different Python versions, so lead to a bunch of failed jobs here). Some comments:

  • One issue with the tests is that there's a bug in some of the error messages that leads to an error in the test (e.g., https://github.com/jobovy/galpy/actions/runs/9051119773/job/24867266058?pr=643#step:19:382). I could push a fix and you can rebase, but it might be easiest if you just fix this in this PR so we can be sure that it fully fixes the issue. The main issue is that pot should be used instead of p in the error message that fails
  • The error message is only displayed because energy is not conserved well enough. So fixing the error message should allow you to see how big of an issue this is. To iterate on the tests, you might want to run them locally before pushing to GitHub. You can do this with
    pytest -v tests/test_orbit.py -k test_energy_jacobi_conservation -xs
    
    for the main orbit integration tests and similar for the other tests that fail:
    pytest -v tests/test_orbit.py -k test_integrate_negative_time -xs
    
    and
    pytest -v tests/test_orbit.py -k test_integrate_backwards -xs
    

@jobovy
Copy link
Owner

jobovy commented May 15, 2024

Also, to avoid the pre-commit commits here (which you should pull in before making any further changes), install pre-commit locally:

pip install pre-commit
pre-commit install

This will run the pre-commit hooks whenever you commit locally, automatically fix issues (like indentation and code style), and tell you when you need to fix something manually.

@jobovy
Copy link
Owner

jobovy commented May 21, 2024

Thanks for fixing the error message bug! Looks like there's some flakiness in the integrator, because the energy conservation tests pass for some Python/OS versions but fail for others (at least that's what it seems like from the test runs). It seems like the integrator sometimes just returns NaNs. Also the backwards integration tests seem to always be failing, maybe the integrators doesn't immediately work for backwards integration?

@johnwez1
Copy link
Author

I made the PR after every test in test_orbit was passing on local, so yeah I guess something dependant on OS/Python version. I've recreated some issues with a fresh python 3.12 so hopefully that will lead to a fix. Anyway, leave it with me! I don't have a whole lot of free time at the moment to work on it but hopefully it shouldn't take too long.

Copy link

This PR has been automatically marked as stale because it has not had recent activity. Please close the PR if it is no longer relevant. If no further activity occurs, the PR may be closed. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Aug 12, 2024
@hannorein
Copy link

hannorein commented Aug 13, 2024

@dangcpham just pointed out this PR to me. A couple of comments:

  • The pow(*,1/7) function call is most certainly OS/machine dependent and could explain the difference you see in local/CI runs. Have a look at the REBOUND source code for a simple and fast machine independent implementation.
  • I understand why treating each timestep separately without taking data from previous timesteps into account is desirable. However, in that case the performance will be quite quite bad as you'll need many more predictor corrector loops.
  • You might want to switch to the new time stepping criteria that @dangcpham came up with: https://arxiv.org/abs/2404.07160. It should perform better in all cases.

@jobovy
Copy link
Owner

jobovy commented Aug 19, 2024

Thanks for the helpful comments @hannorein!

I'll pin this PR so it doesn't get automatically closed, in case @johnwez1 or someone else wants to finish it soon.

@jobovy jobovy added pinned and removed Stale labels Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants