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

Non finishing ODE integration with Field and detector #290

Closed
Serrof opened this issue Dec 2, 2023 · 5 comments
Closed

Non finishing ODE integration with Field and detector #290

Serrof opened this issue Dec 2, 2023 · 5 comments
Assignees
Labels
field Anything related to interface CalculusField ode regression
Milestone

Comments

@Serrof
Copy link
Contributor

Serrof commented Dec 2, 2023

See Orekit forum post: https://forum.orekit.org/t/regression-with-field-propagation-detection/3108/4

These regressions are due to the use in FieldDetectorBasedEventState of isZero, when integration happens between two Field times whose difference lies beyond the getReal component.

@Serrof Serrof added field Anything related to interface CalculusField ode regression labels Dec 2, 2023
@Serrof Serrof added this to the 3.1 milestone Dec 6, 2023
@Serrof
Copy link
Contributor Author

Serrof commented Dec 9, 2023

For the second regression, the problematic call to isZero seems to be on line 369 (on loopG in if statement). If one replaces is with getReal() == 0.0, the regression is gone

@Serrof
Copy link
Contributor Author

Serrof commented Dec 9, 2023

For the first regression, problem is on line 427 with afterRootG in if statement.

@Serrof
Copy link
Contributor Author

Serrof commented Jan 24, 2024

I'm struggling to create reproductible cases in Hipparchus that would serve as unit tests. I'm already struggling to find short ones in Orekit. @maisonobe you know FieldDetectorBasedEventState better than me, do you event understand exactly why these two lines are problematic when there is a time detection with a non-zero gradient in the independent variable?
I'm almost tempted to make the two changes without any tests checking that the regression doesn't occur again in the future. It's not good practise but FieldDetectorBasedEventState is not a reader-friendly class.

@maisonobe
Copy link
Contributor

You are right, checking the value itself is sufficient, and in fact checking the partial derivatives is wrong here: when we cross a zero, there is no reason why the derivatives should be zero. In fact the derivative with respect to time itself is non-zero.
So +1 on the change, and OK for not adding specific tests, I agree it is difficult.

@Serrof Serrof self-assigned this Jan 25, 2024
@Serrof
Copy link
Contributor Author

Serrof commented Jan 25, 2024

Solved via commit

@Serrof Serrof closed this as completed Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
field Anything related to interface CalculusField ode regression
Projects
None yet
Development

No branches or pull requests

2 participants