Bug #262

OEMParser loads NaN accelerations into PVCoordinates creating propagation problems

Added by Hank Grabowski 10 months ago. Updated 4 months ago.

Status:ClosedStart date:2017-01-17
Priority:HighDue date:
Assignee:Hank Grabowski% Done:

0%

Category:-
Target version:-

Description

The updated OEMParser will fill in Vector.NaN if there is no acceleration data. When being evaluated in event handlers the getPVCoordinates return NaN values. If there is no acceleration then it shouldn't be constructed wiht any.

History

#1 Updated by Evan Ward 10 months ago

new PVCoordinates(p, v) results in an object where the acceleration is all zeros. Zero acceleration is a valid value, so it becomes impossible to tell if acceleration is present and valid or not. Using NaN for acceleration values gives a clear indication if it is valid or not. But as you point out using shiftedBy() on an object with NaN accelerations would result in a PVCoordiantes object that is all NaN. So perhaps just adding documentation to point the user to getAvailableDerivatives() to tell if the acceleration values are valid or not.

#2 Updated by Hank Grabowski 10 months ago

Evan Ward wrote:

new PVCoordinates(p, v) results in an object where the acceleration is all zeros. Zero acceleration is a valid value, so it becomes impossible to tell if acceleration is present and valid or not. Using NaN for acceleration values gives a clear indication if it is valid or not. But as you point out using shiftedBy() on an object with NaN accelerations would result in a PVCoordiantes object that is all NaN. So perhaps just adding documentation to point the user to getAvailableDerivatives() to tell if the acceleration values are valid or not.

The problem is it is deep in the event detector that this occurs. Should PVCoordinates have a boolean flag for "hasAcceleration" which is set depending on which constructor was used?

#3 Updated by Hank Grabowski 10 months ago

I implemented a change based on what I wrote in the dev forum but this was before I read Evan's comment. I'd like to address his concern but maybe that should be done on another branch/issue.

https://www.orekit.org/forge/projects/orekit/repository/revisions/5ad7d09f9d853b4687362d2a1bff6fd2d886d203

#4 Updated by Luc Maisonobe 10 months ago

Perhaps we should directly test for NaNs in the shiftedBy method itself. However, I am on the fence on this: NaN could come
from someone explicitly saying no acceleration is known, so acceleration should be ignored, or they could come from a
computation that fails and NaNs should be propagated. Testing for NaNs only in the acceleration seems a good compromise
to me: it correctly represents the first case (user setting NaN when reading an ephemeris), and would probably not change
anything on the second case: if computation fails, NaNs will most certainly propagate by other means.

#5 Updated by Evan Ward 10 months ago

At this point my preference is for adding documentation instructing the user to check getAvailableDerivatives() for parsed ephemerides before using the velocity or acceleration parts of the returned PVCoordinates. This would free us to fill in zeros for V or A if they are missing which means the shiftedBy() method would work correctly as is, without any additional checks.

All the proposals sound reasonable though if they're well documented.

#6 Updated by Hank Grabowski 10 months ago

The problem with it being a documentation thing is that the manifestation of this for me was that the ElevationDetector stopped working because the PVCoordinates that were handed back to it were always NaN. We will have to scrub all of our detectors, and potentially other code, and expect the developers to do so in their own code if we go that route.

I should be able to load an ephemeris file that didn't have acceleration embedded and run event detectors, et cetera. I can't do that under Orekit 9.0 right now. That is a major problem for me.

#7 Updated by Evan Ward 7 months ago

I believe this is fixed in 5ad7d09 and 39a5b1e

Hank, can you confirm this is working as you expect now?

#8 Updated by Luc Maisonobe 6 months ago

Can we set this as solved?

#9 Updated by Luc Maisonobe 6 months ago

  • Status changed from In Progress to Resolved

#10 Updated by Luc Maisonobe 4 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF