search

Hipparchus-Math / hipparchus

An efficient, general-purpose mathematics components library in the Java programming language
Apache License 2.0
139 stars 41 forks source link

Non finishing ODE integration with Field and detector #290

Closed Serrof closed 8 months ago

Serrof commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

Serrof commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

Solved via commit