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

Remove deprecated Orbit.from_body_ephem and its tests #1110

Merged

Conversation

ishanSrt
Copy link
Contributor

Closes #1095

@ishanSrt ishanSrt temporarily deployed to validation-env February 14, 2021 17:15 Inactive
@codecov
Copy link

codecov bot commented Feb 14, 2021

Codecov Report

Merging #1110 (6a7befe) into main (3c1d0eb) will decrease coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1110      +/-   ##
==========================================
- Coverage   90.02%   89.63%   -0.40%     
==========================================
  Files          73       73              
  Lines        3861     3870       +9     
  Branches      330      332       +2     
==========================================
- Hits         3476     3469       -7     
- Misses        297      313      +16     
  Partials       88       88              
Impacted Files Coverage Δ
src/poliastro/plotting/_base.py 87.83% <100.00%> (+0.78%) ⬆️
src/poliastro/twobody/orbit.py 79.64% <0.00%> (-3.54%) ⬇️

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 3c1d0eb...6a7befe. Read the comment docs.

@ishanSrt
Copy link
Contributor Author

ishanSrt commented Feb 14, 2021

Should I add a decorator to the deprecated function and add it to the exclude_lines for coveragerc @astrojuanlu ?

@astrojuanlu
Copy link
Member

Hello @ishanSrt , sorry for the delay! Are you able to see the Quality failures? In the logs, I see this:

2021-02-14T17:19:12.4724784Z check run-test: commands[0] | flake8 src tests
2021-02-14T17:19:16.2979791Z tests/tests_twobody/test_orbit.py:21:1: F401 'poliastro.bodies.Pluto' imported but unused
2021-02-14T17:19:16.2980742Z tests/tests_twobody/test_orbit.py:40:1: F401 'poliastro.frames.equatorial.ICRS' imported but unused
2021-02-14T17:19:16.2981616Z tests/tests_twobody/test_orbit.py:55:1: F401 'poliastro.warnings.TimeScaleWarning' imported but unused
2021-02-14T17:19:16.3174916Z ERROR: InvocationError for command /home/vsts/work/1/s/.tox/check/bin/flake8 src tests (exited with code 1)

please address this proactively so we can focus on the interesting parts of the review :)

@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 775c916 to 2d3b36b Compare February 18, 2021 21:38
@ishanSrt ishanSrt temporarily deployed to validation-env February 18, 2021 21:38 Inactive
@astrojuanlu
Copy link
Member

Interestingly, the image tests failed, which suggests that there are some pixel differences. @ishanSrt do you know how to investigate those?

@ishanSrt
Copy link
Contributor Author

I don't @astrojuanlu but can you point me to some documentation? and I'll look

@ishanSrt
Copy link
Contributor Author

Interestingly, the image tests failed, which suggests that there are some pixel differences. @ishanSrt do you know how to investigate those?

also they aren't faling locally on my machine, I'll have to add the CI to my repo and spit the output out somewhere, for debugging

@astrojuanlu
Copy link
Member

@ishanSrt If they're failing on CI but not locally, perhaps you have to rebuild your tox environment. Something like:

$ tox -e -r py38-images

(only do -r once to save time)

Can you confirm whether this passes or breaks?

@astrojuanlu
Copy link
Member

Also, this will need a rebase

@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 2d3b36b to 2e5b219 Compare March 4, 2021 18:26
@ishanSrt ishanSrt temporarily deployed to validation-env March 4, 2021 18:26 Inactive
@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 2e5b219 to 9cb100c Compare March 4, 2021 19:48
@ishanSrt ishanSrt temporarily deployed to validation-env March 4, 2021 19:48 Inactive
@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 9cb100c to 41b0305 Compare March 4, 2021 21:30
@ishanSrt ishanSrt temporarily deployed to validation-env March 4, 2021 21:30 Inactive
@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 41b0305 to 9eb72ad Compare March 4, 2021 21:31
@ishanSrt ishanSrt temporarily deployed to validation-env March 4, 2021 21:31 Inactive
@ishanSrt ishanSrt force-pushed the remove-deprecated-Orbit.from_body_ephem branch from 9eb72ad to 6a7befe Compare March 4, 2021 21:56
@ishanSrt ishanSrt temporarily deployed to validation-env March 4, 2021 21:56 Inactive
@ishanSrt
Copy link
Contributor Author

ishanSrt commented Mar 4, 2021

Should I add a decorator to the deprecated function and add it to the exclude_lines for coveragerc @astrojuanlu ?

@astrojuanlu please review

Base automatically changed from master to main March 11, 2021 09:11
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 for this @ishanSrt and sorry for the long delay 🙏🏽 This can be merged already.

@astrojuanlu astrojuanlu merged commit 7360225 into poliastro:main Mar 20, 2021
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.

Remove internal uses of deprecated Orbit.from_body_ephem
2 participants