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

Add angular speed and rotational period #746

Merged
merged 6 commits into from Aug 16, 2019

Conversation

@Sedictious
Copy link
Member

Sedictious commented Aug 1, 2019

No description provided.

Copy link
Member

Juanlu001 left a comment

Left some comments about the rotational velocities and periods.

src/poliastro/constants/rotational_elements.py Outdated Show resolved Hide resolved
src/poliastro/constants/rotational_elements.py Outdated Show resolved Hide resolved
Copy link
Member

Juanlu001 left a comment

Left some final comments, almost good to go!

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 11, 2019

Copying a comment I left in an outdated review (oh well... I sometimes mess up with this)

Yes, if these are derived values let's not include them in constants, and only the rotational periods. Then we could have a property like body.angular_speed that would do the required operation. This way we keep poliastro.constants only with stuff we can check in a book, a paper or similar.

On the other hand, this will need a rebase.

@Sedictious Sedictious force-pushed the Sedictious:constantspkg branch from a804a8b to e0344cf Aug 16, 2019
@Sedictious Sedictious force-pushed the Sedictious:constantspkg branch 2 times, most recently from 7d7a6b2 to fc516c1 Aug 16, 2019
@Sedictious Sedictious force-pushed the Sedictious:constantspkg branch from fc516c1 to f9ff4f9 Aug 16, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 16, 2019

Ready to merge when the tests pass!

@codecov

This comment has been minimized.

Copy link

codecov bot commented Aug 16, 2019

Codecov Report

Merging #746 into master will increase coverage by 0.08%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   87.49%   87.57%   +0.08%     
==========================================
  Files          55       57       +2     
  Lines        3223     3252      +29     
  Branches      279      279              
==========================================
+ Hits         2820     2848      +28     
- Misses        327      328       +1     
  Partials       76       76
Impacted Files Coverage Δ
src/poliastro/constants/general.py 100% <ø> (ø)
src/poliastro/constants/__init__.py 100% <100%> (ø)
src/poliastro/constants/rotational_elements.py 100% <100%> (ø)
src/poliastro/bodies.py 68.19% <92.85%> (+1.39%) ⬆️

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 b2a51ae...f9ff4f9. Read the comment docs.

@Juanlu001 Juanlu001 merged commit 8675f04 into poliastro:master Aug 16, 2019
9 checks passed
9 checks passed
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
codecov/patch 96.55% of diff hit (target 87.49%)
Details
codecov/project 87.57% (+0.08%) compared to b2a51ae
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Aug 16, 2019

Merged, thanks a lot!

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.