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

Add some more tests and checks #1255

Merged
merged 9 commits into from
Jun 20, 2021

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented Jun 18, 2021

This pull request is a continuation of addition of some tests and checks.

  • The altitude checks were added to prevent creation of an orbit with negative altitude. This might yield unexpected results at some places, for example, in the lithobrake event.

Questions

  • It looks like @pytest.mark.skipif("czml3" not in sys.modules, reason="requires czml3") gets repeated for each test in test_czml.py. Could we instead have a global pytestmark to skip the whole module if czml3 is not installed?
pytestmark = pytest.mark.skipif("czml3" not in sys.modules)
  • ALL_PROPAGATORS in twobody/propagation.py seem to not contain markley and danby. This seems to be contrary to the name of the variable. Is this expected? If not, should we include all the propagators in ALL_PROPAGATORS?

  • In Some test additions #1253, you had advised to make expected_doc as a fixture. It seems that there are more expected_doc's inside the tests. Is it necessary to have a fixture for each of them?

@astrojuanlu Could you let me know what do you think about the above questions?

Thanks!

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #1255 (8c5a9c7) into main (0a0c005) will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1255      +/-   ##
==========================================
+ Coverage   91.47%   91.67%   +0.19%     
==========================================
  Files          79       79              
  Lines        4143     4144       +1     
  Branches      362      364       +2     
==========================================
+ Hits         3790     3799       +9     
+ Misses        266      261       -5     
+ Partials       87       84       -3     
Impacted Files Coverage Δ
src/poliastro/twobody/propagation.py 91.48% <ø> (-0.18%) ⬇️
src/poliastro/twobody/orbit.py 83.17% <100.00%> (-0.12%) ⬇️
src/poliastro/core/propagation/__init__.py 96.00% <0.00%> (+1.66%) ⬆️
src/poliastro/czml/extract_czml.py 92.85% <0.00%> (+3.17%) ⬆️

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 0a0c005...8c5a9c7. Read the comment docs.

@jorgepiloto
Copy link
Member

The logic when building the ALL_PROPAGATORS macros is wrong! 😱

We are only including the propagators which are shared among the elliptic, parabolic and hyperbolic ones. Thank you for noticing this @Yash-10! 🚀

@jorgepiloto
Copy link
Member

This could be fixed by doing:

ALL_PROPAGATORS = set(ELLIPTIC_PROPAGATORS + PARABOLIC_PROPAGATORS + HYPERBOLIC_PROPAGATORS)

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 18, 2021

Thanks @jorgepiloto for your answer! Also, while we were on that module, I noticed that the "Note" inside cowell's docstring mentioned poliastro.integrators. I thought it was removed some time back and I hence changed the docstring to mention scipy's solve_ivp.

@Yash-10 Yash-10 force-pushed the Add-some-more-tests-and-checks branch from 0010d81 to 668b97c Compare June 18, 2021 17:38
Slight change in docstring

Format table
@Yash-10 Yash-10 force-pushed the Add-some-more-tests-and-checks branch from 668b97c to 3859842 Compare June 18, 2021 17:42
Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

Just some suggestions. After those, this is ready to be merged by my side 👍🏽

src/poliastro/twobody/propagation.py Outdated Show resolved Hide resolved
tests/tests_twobody/test_propagation.py Show resolved Hide resolved
@astrojuanlu
Copy link
Member

About having a global marker: yes, absolutely! I didn't know having a pytestmark would "apply marks at the module level" https://docs.pytest.org/en/6.2.x/example/markers.html#marking-whole-classes-or-modules

In #1253 I suggested to make expected_doc a fixture in case we wanted to reuse it for more than 1 test. If not, every test can have their own expected_doc and that's fine.

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.

This looks good to me! Ready to merge, unless @Yash-10 wants to make some extra additions (like the global pytest marker)

@astrojuanlu
Copy link
Member

(And pending on the comment @jorgepiloto left)

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 20, 2021

Thanks @astrojuanlu I added the global pytest marker.

Copy link
Member

@jorgepiloto jorgepiloto left a comment

Choose a reason for hiding this comment

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

All modifications looking good, approving this 👍🏽

@jorgepiloto jorgepiloto merged commit 8aca71a into poliastro:main Jun 20, 2021
@Yash-10 Yash-10 deleted the Add-some-more-tests-and-checks branch June 20, 2021 17:54
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