Skip to content

Conversation

@The9Cat
Copy link
Member

@The9Cat The9Cat commented Apr 16, 2025

Summary

This is a fix for the PR in #99: IntegrateOrbits() returning time series that DO NOT include the final time point.

Details

The patch condenses the previous patch from PR #99 to only changes relevant to the tfinal issue. Specifically, the features are:

  • Checks on the time interval and sign of the step size for consistency, as suggested by @M1ssing-N0
  • Bounds checking ensures that the initial and final times provided by the user are in the returned orbit
  • By default, the step size is decreased slightly, if necessary, to achieve an even multiple of steps in the provided interval
  • If nout is provided, the algorithm computes a new step size that allows a uniform integral stride of some step size equal or smaller than the provided step size while providing exactly nout points in the return.

@michael-petersen
Copy link
Member

Confirming that this builds fine; is there a specific test case you have been running to verify? (And/or are there any examples that need updating?)

I now see tfinal outputs when I use IntegrateOrbits(), so this looks good to me.

@The9Cat
Copy link
Member Author

The9Cat commented Apr 17, 2025

I used orbits in an EXP field.ipynb from EXP-code/pyEXP-examples/Tutorials/Orbits and ran through an array of initial and final points and different limiting nout values to check limiting cases. E.g. the interval [0.0, 0.2] or [0.2, 0.0] with different step sizes and values of nout. Also checked impossible values, like [0.2, 0.0] with a positive step size to check the sanity checks.

One final question is whether IntegrateOrbits() should be allowed to adjust the step size provided by the user. Currently it does. E.g. if you provide [0, 0.2], with a step size of 0.0133, it will change the step size to 0.013333333... to provide 150 evenly spaced intervals and 151 return points, including the end points. Similarly, if you provide [0, 0.2] and want 40 points, you will get 40 points with an internal computation with h=0.00128205 and a stride=4. If you ask for 300 points, it will only give the original 150 with the adjusted step size.

The questions here are:

  1. Is changing the step size to make even intervals what we want? Or should there be one arbitrarily shorter last step with most steps on the requested step size h? I prefer even steps I think.
  2. Right now, I taking nout to be guideline. E.g. if the provided step size gives more intervals naively, that's what you get. It only strides to give a smaller number of outputs than (tfinal - tinit)/h. I've gone back and forth on this.

@M1ssing-N0
Copy link

Also tested this build on some code similar to the example.

I tried the test case above (range [0, 0.2], step size 0.0133), and h is correctly adjusted.
However, when I tried a test case where the range can be divided by the step size with no remainder (for example, [0, 0.2] with 0.01), the code still tries to adjust h and output one less point (instead of 20 intervals and 21 data points, it returned 19 intervals and 20 points). Have you seen similar behavior?

@The9Cat
Copy link
Member Author

The9Cat commented Apr 17, 2025

I didn't notice that, but round off will most certainly make the algorithm do that. In your case h=0.0099999 will give 20 intervals and 21 output points and h=0.0100001 will give 19 intervals and 20 points. Is this a problem do you think?

Perhaps a better choice would be to provide a buffer in the rounding so that a user that tries your example will get a consistent result? I'll do that.

As an alternative, and this is one of my questions above, I could easily change the algorithm to enforce returning exactly nout points if nout is specified. Right now, it will return exactly nout points if nout is less the number computed from the step size. Otherwise, if nout is larger than this number, it returns the number computed from the step size. I'm also inclined to make this change.

@The9Cat
Copy link
Member Author

The9Cat commented Apr 17, 2025

The last commit has two minor changes:

  1. Fixes the issue from @M1ssing-N0 by preventing round off close to a rounding ceiling boundary by using the middle of the interval as the rounding point.
  2. Changes the algorithm so that the specification of nout returns nout samples of the orbit, equally spaced in time without fail. Internally, the minimum value for nout is 2. If the user specifies nout=1, it gets changed to two, and the two end points are returned.
    Let me know if you find additional isssues. Otherwise, I'm ready to call it done.

@M1ssing-N0
Copy link

I've tested the latest build. I agree it's good to go!

@michael-petersen michael-petersen merged commit 8b18297 into main Apr 19, 2025
4 checks passed
@michael-petersen
Copy link
Member

Noting here that we'll want to bump the version when merging #124.

@michael-petersen michael-petersen deleted the tfinalFix branch April 25, 2025 19:07
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