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

including tfinal when integrating an orbit #99

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

Conversation

M1ssing-N0
Copy link

Currently, the time of the last sample is (tfinal-tstep) instead of tfinal.
I edited the code to extend the integration until tfinal which may be more intuitive.
I also include more sanity check (e.g., tfinal>tinit) for better quality of life.

Commit 12176dc: Change names of Eigen::Tensor-->ndarray generators for clarity
@michael-petersen
Copy link
Member

Thanks for getting started on this!

I understand that the goal here is a use case where you have finer control to manually change the basis at some larger time step (i.e. sequence several IntegrateOrbits calls together), and right now the effective dt=(tfinal-tinitial)/h results in the orbits not being integrated to have an output at tfinal. Is that right?

Looking through, this will create some breaking changes, so I think we'll definitely want to think about whether this is the best way to implement the goal. We also probably want to have some defaults rather than nullopt, although the catches now will help with that.

@The9Cat
Copy link
Contributor

The9Cat commented Dec 16, 2024

This is an interesting approach, but we are very reluctant to change the IntegrateOrbits API. As Mike said, it will break existing scripts and applications. It would be better, at this point, to fix the internal step counts to ensure that the last point is tfinal. This was the intent of the code as written. Specifically, the goal of lines 3856-3865 is choose a step size that starts at tinit and ends exactly at tfinal (up to machine precision at least). If that is broken, let's fix that first.

@M1ssing-N0
Copy link
Author

Thanks for your comments! I agree that I am making some aggressive changes like introducing nullopt.
The changes I made serve to:

  1. Fix the missing output at tfinal (e.g., for tinit=0, tfinal=1, tstep=0.1, it will only integrate from 0 to 0.9), as mentioned by Michael,
  2. Provide more consistency and sanity check for better quality of life,
  3. Enable simultaneous input of h and nout, which is the reason for introducing optional arguments.
    Maybe I can focus on the first to objectives first? I can revert back the changes to arguments h and nout

This reverts commit c0bfbcd, reversing
changes made to a47595d.
This partially reverts commit 2336b9a.

Updated sanity check to allow negative tstep
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.

3 participants