Weird…
There’s a slight difference between the two RMS at iteration 0, which means the propagation itself is slightly different.
My major concern is not this difference itself but the fact that we need twice the number of iterations to converge. There must have been a numerical instability introduced somewhere.
Not sure I’ll have time to retrieve the code and look into it thoroughly this week.
I will let all the clone() methods to leave unchange the test values.
But this investigation makes us think that cloning a double change values at a numerical precision : preventing developers from editing arrays values… is editing arrays values… Interesting, isn’t it ?
I really don’t think cloning changes double values.
Maybe we clone because other parts of the code change the values in the returned array and we clone for safety reasons.
I see. Well I’m rather against introducing unnecessary discrepancies between Field and non-Field stuff. So I’d say the additional contribution cannot wait for another release no, as I believe it would be API breaking. Had I understood that you guys were not gonna do the Field as well I would have suggested to wait for 14.0 or otherwise come up with a plan. Anyway…
I think we should be happy of any “external” (external = from a user and not from an official commiter) contribution. Especially if it is welcomed by the community. I find your message can discourage users from doing new contribution.
The non-field implementation of AdditionalDataProvider, didicated to orbit propagation, will be used 99.9% percent of the time compared to a Field version. Anyway…
I am particularly grateful for external contributions as you call them, my bad for not emphasizing it more. It’s just that this one is actually rather big and as such has more consequences than a smaller one.
The Field stuff is a pain in the neck due to the fact that the langage doesn’t support operator overload (what if instead of Java 21 we went for Kotlin?). Most of it is under the hood tho in Orekit and the user only deals with what they have to. It’s thanks to all the efforts that have been put in the library development over the years, starting way before my own involvement. This is why I’d appreciate if it wasn’t just the burden of committers to make it so. But I’m probably a bit naive and asking for too much, I guess it’s my fault that I didn’t ask in advance what was going to become of FieldAdditionalDataProvider and its friends.
Of all people, I know too well how much work is needed to make a significant contribution like Anne-Laure’s one. To be honest I’ve done a lot for 13.0 and once it’s released I’ll probably take a break. Let’s be clear that I certainly don’t want to deter people from contributing. As a matter of fact I want to help them, and planning ahead for the present one is part of this support.
Going back on the topic. I’ve a small question. Anne-Laure preserved the AdditionalStateProvider class in order to limite the impact of 12 to 13 upgrade for users having a lot implementation of that interface.
Looking at the code, I find it a little bit confusing to have both AdditionalStateProvider and AdditionalDataProvider.
What do you think about removing AdditionalStateProvider?
Last but not least, if we aggree with removing AdditionalStateProvider, I’ll of course do the change in the Field part to have the same names in interfaces and methods in Propagator and SpacecraftState.
Because having AdditionalData only for non-field and AdditionalState for field will be extremely confusing
First of all thanks for following up with this.
About the second impact, the thing is that users will have additional states not just when they’ve used an AdditionalStateProvider, but also when using AdditionalDerivativeProvider. And I’m also not sure how such cast would work with the Python wrappers. If we remove AdditionalStateProvider as you suggest, could we keep a convenience method getAdditionalState in SpacecraftState that will look up if there is a Double instance under than name and return it?
?
