Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Avoid unnecessary element conversions in propagation #921

Closed
astrojuanlu opened this issue Apr 19, 2020 · 6 comments
Closed

Avoid unnecessary element conversions in propagation #921

astrojuanlu opened this issue Apr 19, 2020 · 6 comments

Comments

@astrojuanlu
Copy link
Member

Summary:

# FIXME: Refactor this insanity to remove a lot of back and forth
# 1. Convert to classical to extract nu values (either 1 or 3 are no ops)
# 2. Sample values of nu -> M -> times
# 3. Convert to cartesian to call the farnocchia propagator harcoded here (either 1 or 3 are no ops)
# 4. For each time, convert to classical to retain "slow" elements
# 5. For each time, compute nu0 -> M0 (same value always)
# 6. For each time, t -> M -> nu
# 7. For each time, convert classical to cartesian and assemble orbit

At the moment, all propagators are forced to receive cartesian elements and return cartesian elements. This introduces inaccuracies, especially given the fact that most propagators work internally with classical orbital elements.

We should find a way to introduce more intermediate layers so:

  • At the highest level, Orbit.propagate and Orbit.sample work exactly the same
  • At the intermediate level, the function poliastro.twobody.propagation.propagate can still receive any method and produce a CartesianRepresentation object
  • At the lowest level, I have a way to call the propagators based on classical elements without intermediate conversions
@astrojuanlu
Copy link
Member Author

I'm wondering if delta_t_from_nu and nu_from_delta_t would be good enough for all the other propagators...!

@astrojuanlu
Copy link
Member Author

Prospective contributors: notice that most of the propagators in https://github.com/poliastro/poliastro/tree/master/src/poliastro/core/propagation do something like this:

@jit
def propagator_name(k, r0, v0, tof):
    p, ecc, inc, raan, argp, nu = rv2coe(k, r0, v0)
    ...
    return coe2rv(k, p, ecc, inc, raan, argp, nu)

for all of them, create new functions that do

@jit
def propagator_name_coe(k, p, ecc, inc, raan, argp, nu):
    ...
    return nu

so we can replace the current ones with

@jit
def propagator_name(k, r0, v0, tof):
    p, ecc, inc, raan, argp, nu = rv2coe(k, r0, v0)
    nu = propagator_name_coe(k, p, ecc, inc, raan, argp, nu)
    return coe2rv(k, p, ecc, inc, raan, argp, nu)

Also, notice that https://github.com/poliastro/poliastro/blob/master/src/poliastro/core/propagation/farnocchia.py, but no https://github.com/poliastro/poliastro/blob/master/src/poliastro/core/propagation/markley.py, https://github.com/poliastro/poliastro/blob/master/src/poliastro/core/propagation/vallado.py... therefore, when doing this change, consider creating new modules for the propagators currently in https://github.com/poliastro/poliastro/blob/master/src/poliastro/core/propagation/__init__.py.

@astrojuanlu
Copy link
Member Author

@andrea-carballo expressed interest on working on this! 🚀 let's start with farnocchia first

@astrojuanlu
Copy link
Member Author

In #1026, we added {farnocchia,mikkola,markley,pimienta,gooding,danby}_coe functions, therefore laid the first blocks towards simplifying Orbit.propagate. However, we will need to make poliastro.twobody.propagation.propagate smarter now, by adding some sort of mapping that tells whether the function expects classical elements or cartesian elements.

@astrojuanlu
Copy link
Member Author

Postponing this to the next release.

@astrojuanlu astrojuanlu removed this from the 0.16 milestone Oct 17, 2021
@astrojuanlu astrojuanlu added this to the 0.17 milestone Jan 16, 2022
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jun 29, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 3, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 3, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 4, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 4, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 7, 2022
Fix poliastrogh-921.

Still missing proper sampling support and incompatibilities with cowell.
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 7, 2022
@astrojuanlu
Copy link
Member Author

gh-1521 addresses this by refactoring the propagators into classes. Such classes have a propagate method that receives a state and returns a new state, performing the minimum amount of conversions possible.

@astrojuanlu astrojuanlu moved this from To do to In progress in Refactor propagators Jul 7, 2022
astrojuanlu added a commit to astrojuanlu/poliastro that referenced this issue Jul 8, 2022
Refactor propagators automation moved this from In progress to Done Jul 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

No branches or pull requests

2 participants