While comparing ForceModel and DSSTForceModel I noticed a lot of duplications and I was wondering if they shouldn’t be taken care of for version 12.
ParameterDriver-related methods could go up in interface ParametersDriversProvider (this could also reduce duplications in DiscreteTroposphericModel and IonosphericModel.
init and get(Field)EventDetectors could be gathered in an intermediate interface CommonForceModel
Regarding that last point, numerical force models return a Stream of event detectors while DSST force models return an array. Is there a reason for that or is it just historical?
Also getFieldEventsDetectors has a default implementation in numerical force models but not in DSST force models. Again, is there a reason for that?
I’m not a big fan of having an intermediate interface. In my option, an interface shall represent the purpose of its implementations and have as less as possible methods. With only get{Field}EventDetectors methods inside, I don’t understand that the purpose of CommonForceModel is providing common methods related to force model.
However, that’s a very good idea to put get{Field}EventDetectors in a separated interface which purpose is providing events detectors. For instance, EventsDetectorsProvider, or something else.
For the init methods, I prefer keep it in the two interfaces because I’m not 100% sure that the signature is identical between DSST and Numerical. Currently yes but not sure in the future.
Probably historical
There is no reason. It looks like a omission when time stamped parameter driver were added.
I think I switched from array (or perhaps List) to Stream so callers are aware the return value is built on call (because streams are not reusable), but I guess the intent was too cryptic to be really understood.
Perhaps we should return an Iterable instead? Indeed, the only top level caller for this method is the constructor of the private inner class Main in NulericakOropagator, and it uses foreach on the Stream, which is also available on Iterable.
We should also notify in the javadoc that this method is not intended to be called several time, only once
by the numerical propagator, as it has a side effect of rebuilding the events detectors when called.
here’s my point of view after some thinking: there’s two main approaches to dynamical models. The first one is to quantify forces, as they appear in Newton’s second law. That’s what the current ForceModel does via the acceleration methods. Then, there’s how force models appear in propagators, which can be through a heavy layer of math on top of just physics, as with averaging techniques like DSST. And methods like init and get(Field)EventDetectors are for propagation purposes only. So actually Bryan’s proposal of a dedicated interface for that kind of stuff makes sense. But it could be called DynamicalModel or something rather than only referring to the detectors, of course it depends on what exactly gets moved there. Anyhow, thanks Maxime for dusting off bits of code and aspiring for better overall sense.
Although I understand and agree with your point I think the interface will only have the get(Field)EventDetectors methods. So for now I’m inclined to call it EventDetectorsProvider since DynamicalModel would be a bit too cryptic for the current usage.
But that can change in the future.
Actually, get{Field}EventDetectors default implementations use ParametersDriversProvider#getParametersDrivers() method so EventsDetectorsProvider will have to extend ParametersDriversProvider and both force models will extend EventsDetectorsProvider.
In the end, it will reduce to some sort of common interface… I’m not sure how we can do it differently
I think that TLE and BrouwerLyddanePropagator could also implement the ParameterDriverProvider interface.
Did you tried with different signatures of get{Field}EventDetectors method (i.e., one with parameter drivers and another one without)?
I’m not sure it’s possible, is just an idea
Not sure I understand this one @bcazabonne
Could you have a quick look at branch issue-1167 and interface EventDetectorsProvider and tell me how you would change it? (NB: this is a WIP branch)
