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

Implement improved algorithm for rv2coe. Addresses #566 #567

Merged
merged 5 commits into from Mar 30, 2019

Conversation

Projects
None yet
3 participants
@helgee
Copy link
Contributor

helgee commented Feb 6, 2019

Fixes #566

I have implemented my combined algorithm and added a few smoke tests for all orbit types.

@helgee helgee force-pushed the helgee:rv2coe branch 2 times, most recently from 1142bae to 2f503db Feb 6, 2019

@helgee

This comment has been minimized.

Copy link
Contributor Author

helgee commented Feb 6, 2019

Is there a convenient way to run the toolchain (black, isort, etc...), locally?

And I am giving up for today...
The bloody thing worked locally but now with Anaconda reinstalled it's broken 🙈

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Feb 6, 2019

Is there a convenient way to run the toolchain (black, isort, etc...), locally?

No, and I admit there should be. For the moment, you can do isort -rc src/ && black src/.

Good luck with that Anaconda installation 😅

@helgee

This comment has been minimized.

Copy link
Contributor Author

helgee commented Feb 6, 2019

Makefile? 🤪

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Feb 6, 2019

I was thinking about tox actually: #509 (inspired by https://blog.ionelmc.ro/2014/05/25/python-packaging/)

@helgee helgee force-pushed the helgee:rv2coe branch 2 times, most recently from f6b2ca7 to f887a1a Feb 8, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Feb 10, 2019

When #576 is merged, you will be able to do tox -e check to check all the quality steps.

Also, do you need help for this or just run out of patience? It will need a rebase in any case.

@helgee

This comment has been minimized.

Copy link
Contributor Author

helgee commented Feb 10, 2019

I don't have the patience right now...

Will get back to it soon!

@Himanshu-Garg

This comment has been minimized.

Copy link
Contributor

Himanshu-Garg commented Mar 18, 2019

sir, i can help in rearramging it.
if it doesn't bother you...

thank you

@helgee helgee force-pushed the helgee:rv2coe branch from f887a1a to 417b9fd Mar 29, 2019

helgee added some commits Mar 30, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #567 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #567      +/-   ##
=========================================
+ Coverage   84.87%   85.1%   +0.23%     
=========================================
  Files          51      51              
  Lines        2446    2458      +12     
  Branches      191     192       +1     
=========================================
+ Hits         2076    2092      +16     
+ Misses        319     316       -3     
+ Partials       51      50       -1
Impacted Files Coverage Δ
src/poliastro/core/elements.py 100% <100%> (+6.66%) ⬆️

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 c8f316c...3c313a6. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 30, 2019

Codecov Report

Merging #567 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #567      +/-   ##
=========================================
+ Coverage   84.87%   85.1%   +0.23%     
=========================================
  Files          51      51              
  Lines        2446    2458      +12     
  Branches      191     192       +1     
=========================================
+ Hits         2076    2092      +16     
+ Misses        319     316       -3     
+ Partials       51      50       -1
Impacted Files Coverage Δ
src/poliastro/core/elements.py 100% <100%> (+6.66%) ⬆️

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 c8f316c...3c313a6. Read the comment docs.

@helgee

This comment has been minimized.

Copy link
Contributor Author

helgee commented Mar 30, 2019

Alright! CI is finally all green 😅

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 30, 2019

@Juanlu001
Copy link
Member

Juanlu001 left a comment

The code looks perfect!

If we use this opportunity to settle on -pi <= nu < pi (did I understand correctly) perhaps we should discuss whether we should normalize user data on input and emit a warning. I will open an issue for that.

@Juanlu001 Juanlu001 merged commit d4806c8 into poliastro:master Mar 30, 2019

10 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_py35 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 All good!
Details
codecov/patch 100% of diff hit (target 84.87%)
Details
codecov/project 85.1% (+0.23%) compared to c8f316c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 30, 2019

Awesome @helgee 🎉

@helgee helgee deleted the helgee:rv2coe branch Mar 30, 2019

@helgee

This comment has been minimized.

Copy link
Contributor Author

helgee commented Mar 30, 2019

-pi <= nu < pi (did I understand correctly)

Yup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.