I looked at the new API of IOD methods introduced in 12.0 and I’ve some questions/comments:
@DefaultDataContext
public IodGauss(final double mu, final Frame outputFrame) {
super(mu, outputFrame);
}
IodGauss only has a constructor with @DefaultDataContext. I think it is a big limitation because external software can have their own data context and therefore they wont be able to use this class. I think it is important to add a signature without the @DefaultDataContext.
Same comment for all estimate methods in AbstractAnglesOnlyIod. These methods can be very useful. But having only @DefaultDataContext signatures is a limitation.
Same comment for IodLaplace
/** Constructor.
*
* @param mu gravitational constant
* @param outputFrame orbit output frame
*/
@DefaultDataContext
public IodLaplace(final double mu, final Frame outputFrame) {
super(mu, outputFrame);
}
We now only have a contructor with @DefaultDataContext. In that case, it is a regression compare to 11.3.3.
In 11.3.3 the output frame is in the method signature. Now we shall define the frame in the constructor. Why ?
We can imagine users that would like to compute IOD in different frames without having to instantiate a new IOD algorithm each time.
I think that AbstractAnglesOnlyIod introduced a lot of noise in the different implementations. For instance, we now have to define 3 observers for Laplace. But only 1 is used. So a user will have to perform 3 frame transformations (from topocentric to ECI) for only one used. I understand that it is to have a “generic” abstract method. But is it really generic and necessary?
I also noticed the following one introduced by the AbstractAnglesOnlyIod: IodGauss asks for PVCoordinates of the observer. However, only the position is used. This is a computation time issue because users have to call getTransformTo instead of getStaticTransformTo to compute observer position in satellite frame.
I only quickly looked at the architecture a while ago, and I was also not super satisfied with it. I actually found an error in the javadoc and opened issue #1181.
I think it is actually not an easy task to be generic with IOD, as the algorithms can be quite different but also just the problems quite varied too (angle only, position only, etc.)
So I appreciate the recent efforts to tackle that.
I can’t comment much on the context stuff as I’m not super familiar with it. However, about PVCoordinates, if they can be replaced with something simpler, let’s go for it.
I haven’t looked carefully into the code but I’ll try to do it soon.
I agree with Romain, I appreciate the fact that some efforts were made to try to introduce generics in the IOD method.
However, @bcazabonne, some issues must definitely be fixed asap.
1 and 3. I think these two @DefaultDataContext and the one in IodGooding can be removed, they aren’t needed
2. It is indeed a limitation and generic functions should be added
4. I don’t know why the frame got up in the constructor. It can be put back as input of the method signature easily.
5. Agreed, plus the weirdest thing about this one is that the observer used is the second one and not the first one. I find it completely counter-intuitive.
6. It seems IodGooding has the same issue…
I’ll open an issue and fixe all these issues ASAP. I think they are important because they introduce many regressions in both data context and computation time).
I’ll try to do generic things but if AbstractAnglesOnlyIod introduce too much noise and counter-intuitive things, I’ll remove it (or at least reduce its perimeter).