Simplify State objects - poliastro/poliastro GitHub Wiki
Problem
Target: poliastro 0.6
The twobody
package suffers from circular imports and the plotting code does too much.
Explanation
In 31159b9 the twobody
package was split into modules, leaving a State
class and several specefic subclasses for each representation of a state (position & velocity, classical elements and equinoctial elements). The problem is that putting the imports at the beginning was leading to circular import problems, and therefore they had to be written at the bottom:
https://github.com/poliastro/poliastro/blob/0c6062e/src/poliastro/twobody/core.py#L384
On the other hand, to avoid too many propagations the plotting code is taking advantage of the classical elements to sample the orbit, hence replacing a proper propagation function.
https://github.com/poliastro/poliastro/blob/0c6062e/src/poliastro/plotting.py#L98-L117
This is clearly a hack and a failure to separate concerns, as well as a possible example of premature optimization.
Finally, with the current codebase there is no API to plot orbital segments, resulting in noisy plots that are difficult to read.
Solution
The following steps were planned to solve the problem:
- Decouple
State
so no more circular imports are needed.- Perhaps separate a
State
factory from the base methods that shall be overridden.
- Perhaps separate a
- Move PQW conversion outside of plotting in anticipation of a future framework for reference systems conversion (#108).
- Export
State
propagation as a dataframe or another format that is easy to manipulate.- Maybe an intermediate object will be needed?
- Use this intermediate representation for plotting, thus allowing poliastro to plot just orbital segments.
State
decoupling and Orbit
objects
The State
class, which serves as parent, has two methods, propagate
and apply_maneuver
, that make use of the factory constructors and therefore complicate the separation:
https://github.com/poliastro/poliastro/blob/0c6062e/src/poliastro/twobody/core.py#L325-L363
In fact, after a trivial separation these are the only ones that lead to failing tests:
https://travis-ci.org/poliastro/poliastro/builds/153950540
At this point, some realizations appeared relevant:
- The essence of the desired changes lives in
propagate
andapply_maneuver
, which should have a more complex behavior. - This possibility of increasing complexity might be a sign that they should live elsewhere.
- Both functions share the same philosophy, but maybe
apply_maneuver
should be a generalization ofpropagate
(essentially applying a "delay" maneuver with no impulses, see API-Refactor:-Maneuver-objects for first occurrence of this idea). - The
epoch
of theState
is only used for propagation (and plotting), maybe another sign thatState
is doing too much. - Using just
SkyCoord
objects from astropy is not enough to completely describe the orbit: the position gives information about the present, while the velocity adds information about the future.- This is even clearer when using orbital elements instead of position & velocity vectors.
With this in mind, another object structure using composability arises:
SkyCoord + barycentric velocity = State
State + Time = Orbit
In this way, the State
works as a complete representation of the orbit and the Orbit
[provisional name] would hold the capability to propagate it onto the future.
This separation also removes the cyclic dependency with the propagation code inside the State
objects and simplifies its API, which can be taken as a first step to improve the exporting and plotting capabilities.
Idea: With the current (9c4dd0b) changes, this is the API for creating an orbit:
What about removing the StateFactory
class and move the constructors to Orbit
? This way we don't have to chain two objects and save one import.
References
These resources and codebases served as help or inspiration:
- About proper object design https://glyph.twistedmatrix.com/2016/08/attrs.html
- The refactor of the LTI code in the
scipy.signal
package https://github.com/scipy/scipy/pull/4881 - Explanation of circular import pitfalls https://gist.github.com/datagrok/40bf84d5870c41a77dc6
- The
SkyCoord
object in astropy http://docs.astropy.org/en/v1.2.1/coordinates/skycoord.html - Some issues regarding the support for velocities in astropy https://github.com/astropy/astropy/issues/3544 https://github.com/astropy/astropy/issues/4344 https://github.com/astropy/astropy/pull/5231