Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: propulsion system to use Reals instead of Scalar #353

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

apaletta3
Copy link
Contributor

@apaletta3 apaletta3 commented Feb 15, 2024

After going down a rabbit hole of trying to add python bindings for the physics::data::Scalar class (which currently lacks them) in OSTk physics, I have deemed that much harder than it seems on the surface and come up with an alternative medium term solution.

Context on the cpp side for OSTk physics:

  • There is lots of convoluted code deep down inside of physics::Unit, which would take a non-trivial amount of time to modify if needed
  • the tests for physics::Unit are weak, and the tests for physics::unit::Derived are non-existant (which is scary because we use physics::unit::Derived in a lot of places in astro
  • the tests for data::Scalar and data::Vector and data::Direction are non-existant
  • all derived units should be defined inside of physics::Unit, not as the Thrust and SpecificImpulse and MassFlowRate ones are currently defined inside of PropulsionSystem

Context on the python side for OSTk physics

  • physics::Unit, physics::data::Scalar, physics::data::Vector, and physics::data::Direction are not bound to python at all as classes, and in order to add bindings to physics::data::Scalar, you need to add bindings to physics::Unit, which is not trivial at all due to the aforementioned convolutedness of physics::Unit
  • if data::Scalar and data::Vector were to be bound to python, cpp tests would need to be added for them as well, which would again take time

Context on how the above relates to OSTk astro:

  • ostk::physics::Unit is only used in two places in OSTk astro (AtmosphericDragDynamics (not at the API level) and PropulsionSystem (indeed at the API level)
  • data::Scalar is only used in PropulsionSystem and data::Vector is used nowhere in OSTk astro
  • SatelliteSystem, dynamics, and propagation all do their computations and store their variables in unitless Reals or VectorXds
  • after the bonanza of ostk releases over the past couple days, no more breaking changes 🙏

Path forward:

  • One day I think we will need a complete overhaul of the unit system in OSTk, by either a) completely removing it, b) replacing it with templated compile-time unit checking or boost::units
  • However, in the meantime, and based on all the context above I think the best solution is to remove the use of data::Scalar from PropulsionSystem and store its quantities in SI for 1) consistency with everything else in astro and 2) the fact that you can't use the getters in Python for PropulsionSystem without adding Scalar bindings, and we are going to be using the PropulsionSystem object a lot in the near future in the guidance service and everything related to it

There is a corresponding MR in physics that comments out pybound methods that won't work because they return data::Scalar and data::Vector.

@apaletta3 apaletta3 self-assigned this Feb 15, 2024
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e462123) 89.65% compared to head (6486583) 89.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
- Coverage   89.65%   89.60%   -0.05%     
==========================================
  Files          78       79       +1     
  Lines        7662     7632      -30     
==========================================
- Hits         6869     6839      -30     
  Misses        793      793              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Derollez Derollez left a comment

Choose a reason for hiding this comment

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

Thanks for the investigation details and the cleaning 🙏

@apaletta3 apaletta3 enabled auto-merge (squash) February 16, 2024 11:21
@Derollez Derollez disabled auto-merge February 16, 2024 12:08
@apaletta3 apaletta3 enabled auto-merge (squash) February 16, 2024 12:23
@apaletta3 apaletta3 disabled auto-merge February 16, 2024 12:48
@apaletta3 apaletta3 enabled auto-merge (squash) February 16, 2024 12:51
@apaletta3 apaletta3 disabled auto-merge February 16, 2024 12:57
@apaletta3 apaletta3 merged commit 419c50e into main Feb 16, 2024
12 of 13 checks passed
@apaletta3 apaletta3 deleted the refactor/reduce-reliance-on-units branch February 16, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants