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 coordinates, frames, bodies, everything!!1! #763

Conversation

@Juanlu001
Copy link
Member

Juanlu001 commented Aug 18, 2019

Since we introduced the bodies with static methods I always found them ugly. Finally I took the opportunity to move all the rotation information and the ugliness to poliastro.frames. There is some ugliness we can't escape because astropy frames require some magic to initialize (I have some ideas about how to improve that, but I need more time).

I am not 100 % happy with this pull request though, because I dropped some features along the way, I broke one test, and I didn't check the notebooks. A bit more work is needed, but I'm glad I finally laid down my idea for poliastro.bodies.

def __str__(self):
return f"{self.name} ({self.symbol})"

def __repr__(self):
return self.__str__()

def rot_elements_at_epoch(self, epoch):

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Aug 18, 2019

Author Member

These are the functions that moved

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Aug 18, 2019

Missing:

  • Write down all the features and functions I dropped
  • Check atmosphere - that code uses some custom constants attached to Earth (I think) and will probably break.
  • Think of alternative for bodies with custom properties
    • Adding a dictionary breaks the frame mapping, because we are using the bodies as keys
  • Add at least some smoke tests for the fixed frames
    • Even if the https://github.com/poliastro/validation/ machinery is not ready, we could do a test with GMAT and copy-paste the values
    • Alternatively, we could at least write a test copy-pasting the result from our current implementation, to notice when we change anything
@jorgepiloto

This comment has been minimized.

Copy link
Member

jorgepiloto commented Aug 18, 2019

Old constant values were used in atmosphere for each one of the atmospheric models, so no dependency on bodies has been stablished. This is the reason why tests_atmosphere were passing.

However the plotting.porkchop failed, so I will investigate why 😕

@Juanlu001 Juanlu001 force-pushed the Juanlu001:refactor-coordinates-frames-bodies-everything branch 2 times, most recently from 7702824 to 3def3d3 Aug 18, 2019
@Juanlu001 Juanlu001 force-pushed the Juanlu001:refactor-coordinates-frames-bodies-everything branch from 3def3d3 to 59472a5 Aug 26, 2019
Copy link
Member Author

Juanlu001 left a comment

Left two comments to review later.

@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Aug 26, 2019

One doctest failing:

_______________ src/poliastro/tests/tests_plotting/test_core.py ________________
142: error: "None" has no attribute "template"
@jorgepiloto

This comment has been minimized.

Copy link
Member

jorgepiloto commented Sep 6, 2019

I opened a pull request to @Juanlu001 branch to solve those minor errors in test_perturbations 👍

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #763 into master will decrease coverage by 1.27%.
The diff coverage is 59.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #763      +/-   ##
==========================================
- Coverage   88.46%   87.19%   -1.28%     
==========================================
  Files          60       65       +5     
  Lines        3312     3248      -64     
  Branches      274      273       -1     
==========================================
- Hits         2930     2832      -98     
- Misses        309      343      +34     
  Partials       73       73
Impacted Files Coverage Δ
src/poliastro/constants/__init__.py 100% <ø> (ø) ⬆️
src/poliastro/plotting/porkchop.py 80.32% <ø> (ø) ⬆️
src/poliastro/coordinates.py 100% <ø> (+21.27%) ⬆️
src/poliastro/frames/ecliptic.py 100% <100%> (ø)
src/poliastro/constants/general.py 100% <100%> (ø) ⬆️
src/poliastro/frames/__init__.py 100% <100%> (ø)
src/poliastro/twobody/orbit.py 83.67% <100%> (ø) ⬆️
src/poliastro/frames/enums.py 100% <100%> (ø)
src/poliastro/frames/fixed.py 32.36% <32.36%> (ø)
src/poliastro/frames/equatorial.py 81.48% <81.48%> (ø)
... and 10 more

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 489eb6a...067750d. Read the comment docs.

@Juanlu001 Juanlu001 force-pushed the Juanlu001:refactor-coordinates-frames-bodies-everything branch from e307128 to b6f17d5 Sep 14, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Sep 14, 2019

We will have to apply changes similar to d72bdab to the notebooks as well.

@Juanlu001 Juanlu001 force-pushed the Juanlu001:refactor-coordinates-frames-bodies-everything branch 4 times, most recently from 1c4b04e to 77bd028 Oct 13, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Oct 20, 2019

Should rebase after #785 is merged.

@Juanlu001 Juanlu001 force-pushed the Juanlu001:refactor-coordinates-frames-bodies-everything branch from 77bd028 to 067750d Oct 20, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member Author

Juanlu001 commented Oct 21, 2019

Well well well, the coverage check is failing but this is long overdue. The sooner we land this refactor and start testing it ourselves and feeling comfortable with the new code, the better. We can adjust later.

Merging!

@Juanlu001 Juanlu001 merged commit 417aba8 into poliastro:master Oct 21, 2019
7 of 9 checks passed
7 of 9 checks passed
codecov/patch 59.62% of diff hit (target 88.46%)
Details
codecov/project 87.19% (-1.28%) compared to 489eb6a
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: quality Your tests passed on CircleCI!
Details
ci/circleci: test_py36 Your tests passed on CircleCI!
Details
ci/circleci: test_py37 Your tests passed on CircleCI!
Details
codeclimate Approved by Juan Luis Cano Rodríguez.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Juanlu001 Juanlu001 deleted the Juanlu001:refactor-coordinates-frames-bodies-everything branch Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.