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

feat: add Brouwer-Lyddane Mean Long and Mean Short elements #240

Merged
merged 11 commits into from
Oct 13, 2023

Conversation

vishwa2710
Copy link
Contributor

@vishwa2710 vishwa2710 commented Oct 11, 2023

  • Have to add bindings, will do so after the C++ code is reviewed and any name/flow changes are addressed.
  • Have to add docstrings to C++ code as well

@vishwa2710 vishwa2710 self-assigned this Oct 11, 2023
@vishwa2710 vishwa2710 marked this pull request as draft October 11, 2023 22:57
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #240 (51c8a07) into main (2200a55) will increase coverage by 0.11%.
The diff coverage is 84.55%.

@@            Coverage Diff             @@
##             main     #240      +/-   ##
==========================================
+ Coverage   83.27%   83.39%   +0.11%     
==========================================
  Files          63       66       +3     
  Lines        4886     5505     +619     
==========================================
+ Hits         4069     4591     +522     
- Misses        817      914      +97     
Files Coverage Δ
...els/BrouwerLyddaneMean/BrouwerLyddaneMeanShort.cpp 87.86% <87.86%> (ø)
...t/Models/BrouwerLyddaneMean/BrouwerLyddaneMean.cpp 80.17% <80.17%> (ø)
...dels/BrouwerLyddaneMean/BrouwerLyddaneMeanLong.cpp 90.51% <90.51%> (ø)
...trodynamics/Trajectory/Orbit/Models/Kepler/COE.cpp 79.31% <66.27%> (-3.92%) ⬇️

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.

Huge addition! Might want to add validation tests against GMAT or Orekit? (at least one set of propagated and computed BL values to ensure this is good?)

@Derollez Derollez changed the title feat: add Brouwer-Lyddane Mean Long + Short elements feat: add Brouwer-Lyddane Mean Long and Mean Short elements Oct 12, 2023
Copy link
Contributor Author

@vishwa2710 vishwa2710 left a comment

Choose a reason for hiding this comment

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

Amazing thank you for the feedback. All the unit tests are compared to GMAT so I will put that in the comments.

Copy link
Contributor

@kyle-cochran kyle-cochran left a comment

Choose a reason for hiding this comment

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

Some suggestions to simplify RuntimeError printing.

Will do another, less superficial, review round

Copy link
Contributor

@kyle-cochran kyle-cochran left a comment

Choose a reason for hiding this comment

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

A lot of nit suggestions on adding const-ness to the return types for the getters of the more trivial types (Angle/Length/etc)

@Derollez Derollez marked this pull request as ready for review October 13, 2023 13:18
@vishwa2710 vishwa2710 force-pushed the users/vishwa/add-mean-elements branch from 1e47f44 to 51c8a07 Compare October 13, 2023 21:09
@vishwa2710 vishwa2710 enabled auto-merge (squash) October 13, 2023 21:34
@vishwa2710 vishwa2710 merged commit 8109604 into main Oct 13, 2023
11 checks passed
@vishwa2710 vishwa2710 deleted the users/vishwa/add-mean-elements branch October 13, 2023 21:34
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.

4 participants