as far as naming is concerned, the word “state” should be left for actual continuous variables, I think the generic thing could be “info” or something
let’s not add any more lines of code in (Field)SpacecraftState and use class composition for instance to manage the additional stuff
My opinion is a bit biased since we discussed this afternoon with Anne-Laure about this new feature.
AdditionalStateProvider is a very appreciated feature of Orekit. Extending its purpose is a very nice idea.
Maybe we can directly modify the existing AdditionalStateProvider class? The update will be straighforward for user (i.e. just add <Double> to their implementation). Therefore:
I don’t think that the name of the classe shall be modified. A “state” could be represented by a double value, a String, etc. In fact “state” is already a generic word.
Any additional code will be added in {Field}SpacecraftState. The idea is just to add a generic type to the interface
In the context of dynamical systems, state is kind of a keyword, it’s the vector of dependent variables. The case of user defined ones is already covered by the DoubleArrayDictionnary, and you can’t make that generic.
SpacecraftState has 18 constructors
I just think this deserves some brainstorming
Yes, we are aware of the current complexity of SpacecraftState. Romain did raise alerts several times about this, and I agree with him.
I have no clue how to handle this, but some redesign is probably needed. We really need to think this through for 13.0.
I would suggest something: all developers start by thinking of the pros and cons and if they find they can suggest a new architecture, splitting classes, using polymorphism, using builders… Then in one or two weeks, we set up a live brainstorming online (not really an Orekit Talk, but using similar means), in order to decide something collectively.
And I really like the idea of generalizing additional data (state or other name, I don’t care).
I am struggling with this change… Hope I will end this soon.
I would like to share some questions I am having.
About name : would “AdditionalObjectProvider” fit ?
I am trying to limit modifications to the minimum. Adapting code to a general AdditionalObjectProvider was easy… the most difficult is about the DoubleArrayDictionary. I am trying to define an ObjectDictionary, to be used only in SpacecraftState, limiting impact elsewhere.
There is method that is not much used and causes problem with genericization : org.orekit.utils.DoubleArrayDictionary.View
This exists only to protect from edition.
Do you think I could delete this ? And returns the dictionary itself ?
About the name, I would avoid “object” as for one it is a keyword in Java, and it is also used in business logic in the context of Space Situational Awareness.
I knew the DoubleArrayDictionary was gonna be a pain to generalize. Part of the problem is also that it is used both by the additional state and the additional derivatives.
Maybe open a merge request with your proposed changes, so that people can see more clearly what you’re talking about?
About wording, for me “Information” means a news… maybe “Data” : AdditionalDataProvider ?
About wondering what to modify and how, I was not ready yesterday but I have just pushed my branch. It is NOT ready at all, as it is only compiling : I have ignored tests, javadoc, proper git history … until I am sure this code is in the appropriate mindset.
So I have created a merge request in DRAFT mode, if you want to have a look on ONLY code, please feel free to give me some early feedbacks : Sign in · GitLab
AdditionalDataProvider is fine to me. Don’t know if we should launch a poll for that one
Some of them looks ok, some are more disturbing, especially:
tolerance from 1e-16 to 8e-10 in all tests of KalmanModelTest
Jumping number of iterations/evaluations in BLS and Numerical OD tests
Any idea where it comes from? It looks like the convergence is harder in OD algorithms with this change.
I thought it would be transparent so I’m wondering.
Apart from that, the contributions looked good to me so far!
!