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

[DO NOT MERGE YET] State & orbit array API (iterative implementation) #1492

Draft
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

s-m-e
Copy link
Contributor

@s-m-e s-m-e commented Mar 18, 2022

This PR aims at providing a full OrbitArray API and corresponding tests while relying on a slow iterative implementation based on Orbit first. This should allow the implementation of proper tests before cleanup and refactoring of other package internals begins.

@astrojuanlu astrojuanlu marked this pull request as draft March 18, 2022 10:11
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very high-level comments from our weekly meeting:

  • Let's avoid allocating memory ourselves and accept an out= parameter instead
  • @s-m-e will work on the tests before calling for review
  • Let's break this down into pieces: first, the low hanging fruit (pytest-xdist, decorators going away, etc)
    • Then, we should open another PR for the State signature change (the epoch)

@astrojuanlu
Copy link
Member

While working on gh-1521, I realized it would have been cool to have a StateArray However, progress stalled there because we could not reach full agreement about what to do with the epoch.

In summary: @s-m-e advocates for having it in the State classes, while I was unsure because it's not needed at all for propagation and thus breaks Separation of Concerns. However, if we don't include it, end up with a data structure without times attached to it, which is related to #923. This forces us to either

a. have this information in a higher-level object, potentially introducing small complications in the implementation of some algorithms, or
b. adding it to the State anyway, thus potentially creating a structure that is heavier than needed.

I'll try to devote some thinking to this during SciPy.

@astrojuanlu
Copy link
Member

More arguments for keeping the epochs outside of StateArrays:

  • Creates nice simmetry with Astropy: State objects resemble Coordinate objects from Astropy (which famously don't work with Keplerian elements, see generalize orbital elements machinery from calc_moon astropy/astropy#5067), Orbit and Ephem would be Frame instances from Astropy
  • Creates nice internal simmetry: Orbit = State + epoch, Ephem = StateArray + epoch (which begs the question: to what extent a OrbitArray is needed, but again this deserves further discussion)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants