I had an issue when upgrading a code to Orekit 13.1.X, some Hipparchus exceptions “integration step is too small = 0” popped.
Turns out it came from propagating a FieldNumericalPropagator with a 0s duration, which never lead to an exception before.
In FieldAbstractIntegratedPropagator.integrateDynamics there is an equality check between the date of the initial state and the target propagation date.
If both are equal the method returns the initial state, otherwise the integrator is called.
if (getInitialState().getDate().equals(tEnd)) {
// don't extrapolate
return getInitialState();
}
The equality check is based on method FieldAbsoluteDate.equals().
Before 13.1, only the “date” part of FieldAbsoluteDate was compared.
Since 13.1, the “addendum” is also checked (which is, I think, the proper behaviour).
For some reason (hard to reproduce here), the “date” part of my dates are equal but not the “addendum” part. Which triggered my exception.
That leads me to the real purpose of my topic here: isn’t that only the “date” that we want to check in the code snippet of the propagator above ?
My opinion is that, in this particular case, the “offset” isn’t worth being checked.
So the code should be replaced with:
if (getInitialState().getDate().toAbsoluteDate().isEqualTo(tEnd.toAbsoluteDate)) {
// don't extrapolate
return getInitialState();
}
What do you think ?
Another option would be to use FieldAbsoluteDate.isEqualTo instead of equals, but for some reason the former is simply a call to the latter, so that won’t work.
That leads me to a second question. Since all the helper functions that were implemented for date comparisons (i.e. isCloseTo, isBefore/After/Between etc.) are checking the “date” part only, shouldn’t isEqualTo do the same ?
In the case of propagation shortcut, yes I think comparing only the date part would be fine.
In the general case, I am not sure. I agree the methods isCloseTo and similar really use only the date part too, because they seem to me really targeting ordering concepts, but somehow, I think isEqual is much more stringent on its assumptions, so having it also check the addendum seems logical to me. But this is only my point of view, I would like to see other’s opinions.
For all intend and purposes of that check which is only there for speed in my understanding, yes we could only check on the non Field part. However the check with the addendum should not fail. This is likely linked to an outstanding problem we have with the comparison of Field dates. Basically there’s no proper equivalent to accurateDurationFrom. So shifting Field dates may have non reversible effects for comparison in some circumstances. What do you think @Vincent ?
I’ve thought for a while that isEqualTo is misnamed, even for the non Field version. Java already has the equals method that we can override. What we want to define here is simultaneous occurrence of TimeStampted objects. So the method should be renamed and made more generic in my opinion. For the field version in particular, I would say that it should check the addendum too tho, but I understand why you would want it to just check the AbsoluteDate. However, I don’t think the latter is acceptable if the name remains isEqualTo.
I agree, i’m sot sure we have set up proper test for that.
To provide more information, it is most likely not the case in the observed issue here as we have literally two Field instances that live concurrently.
I also think isEqualTo is misnamed and ambiguous. isSimultaneousTo would be appropriate but it’s a very long word
I think in my case it’s not linked to that. It’s a situation where I use two different fields, one with a fielded-date, the other one without. That’s why the comparator is not happy.
It’s a bit verbose but since it’s much clearer I think we should go for it
Sorry Maxime I misunderstood the problem. If the field dates have same constant part but different addendum, it’s not ok to shortcut the propagation. Because you’re then missing… well, the actual step. For automatic differentiation it wouldn’t work.
I understand. However as you said in the issue, the previous behaviour was probably wrong, and we still have the Hipparchus exception due to the 0 second step.
So I guess investigations are indeed needed
