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

Moving ra, dec, and W calculations to core #1190

Merged

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented May 1, 2021

I noticed the rotational elements were computed in the high-level module. It might be a good choice to move those to core.

Thanks!

@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #1190 (f9b5bc2) into main (dfcce38) will increase coverage by 0.04%.
The diff coverage is 83.47%.

❗ Current head f9b5bc2 differs from pull request most recent head 5ecae3e. Consider uploading reports for the commit 5ecae3e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
+ Coverage   90.27%   90.31%   +0.04%     
==========================================
  Files          76       77       +1     
  Lines        4071     4109      +38     
  Branches      363      363              
==========================================
+ Hits         3675     3711      +36     
- Misses        306      308       +2     
  Partials       90       90              
Impacted Files Coverage Δ
src/poliastro/core/fixed.py 82.29% <82.29%> (ø)
src/poliastro/frames/fixed.py 97.24% <89.47%> (+8.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ae318...5ecae3e. Read the comment docs.

@Yash-10 Yash-10 force-pushed the Fixed-frames-computations-to-core branch from c4d9aea to f9b5bc2 Compare May 1, 2021 13:50
Add docstrings and references

Modify value in MoonFixed
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.

Thanks a lot @Yash-10 ! While I'm not a super fan of having the functions like this, it's something I have struggled with internally for a long time. It's not possible to do it "the smart way" either because each body uses a different equation essentially. And having a list of parameters that are then used in the equations makes everything just more difficult to read. So I am giving my approval on this, since it's an improvement over what we had before.

@Yash-10
Copy link
Member Author

Yash-10 commented May 16, 2021

Sorry @astrojuanlu ! I had intended to convert this into a draft since I suspected some problems in the units. I will fix it soon!

Again, sorry for the trouble!

@astrojuanlu
Copy link
Member

No worries! To clarify, #1229 was not because of this 💪🏽

Right ascension and declination of north pole, and angle of the prime meridian.

"""
Ja = 99.360714 + 4850.4046 * T
Copy link
Member

Choose a reason for hiding this comment

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

This is being computed in degrees and passed in these units to the trigonometric functions below! As pointed out by @Yash-10, the tests within poliastro did not covered all this logic but the validation ones did. Pinging @astrojuanlu, we might discuss this on this week's poliastro meeting (July the 5th, 2021).

Copy link
Member

Choose a reason for hiding this comment

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

The same applies to the rest of the bodies, though this was only noticed for the cases of Jupiter and Moon due to the big amount of terms when computing their rotational elements at epoch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jorgepiloto ! I will soon open a pull request to (hopefully) rectify my mistake!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in general I think having the validation tests is awesome, but also we should have more unit tests :)

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

3 participants