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

Addressing more test warnings #1235

Merged
merged 21 commits into from Jun 11, 2021
Merged

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented May 27, 2021

Continuing work from #1137. Thanks @ishanSrt !

Some analysis

  • The dubious year warnings (coming from test_propagate_long_times_keeps_geometry) due to the dates being far into the future seem to be legitimate, since that test propagates an orbit for 100 years and hence the resultant orbit is mostly going to be out of the non-dubious range defined by erfa.
  • I thought it might be a good choice to just filter the warning since after looking at issue 256, I wondered whether it would be good to still check for long propagations like 100 years. Moreover, reducing the propagation time while still keeping it long enough, say, 60 years didn't seem to solve it.
  • For the second set of erfa warnings, eg:
ERFA function "epv00" yielded 8 of "warning: date outsidethe range 1900-2100 AD"

Any date after about the year 1935 yields that warning. Here, it might be a good choice to just choose a "safe" date value to prevent these warnings pop up.

Thanks, and would love to get any suggestions!


The coverage decrease needs to be addressed and I am working on that...

@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #1235 (7b0aa05) into main (112cba6) will increase coverage by 0.40%.
The diff coverage is 88.88%.

❗ Current head 7b0aa05 differs from pull request most recent head 0be91a0. Consider uploading reports for the commit 0be91a0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1235      +/-   ##
==========================================
+ Coverage   90.82%   91.23%   +0.40%     
==========================================
  Files          78       78              
  Lines        4121     4106      -15     
  Branches      363      360       -3     
==========================================
+ Hits         3743     3746       +3     
+ Misses        288      270      -18     
  Partials       90       90              
Impacted Files Coverage Δ
src/poliastro/czml/extract_czml.py 88.09% <0.00%> (ø)
src/poliastro/twobody/orbit.py 82.83% <ø> (+3.18%) ⬆️
src/poliastro/plotting/misc.py 94.73% <100.00%> (+0.98%) ⬆️

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 112cba6...0be91a0. Read the comment docs.

@Yash-10
Copy link
Member Author

Yash-10 commented May 30, 2021

Points and questions regarding coverage:

  • The test test_misc.py is modified such that no warnings are raised up. Doing this has decreased coverage for misc.py since it now doesn't test for the case when no epoch argument is passed, i.e. the default epoch of J2000 is used which, as per the above analysis, is bound to generate "warning: date outsidethe range 1900-2100 AD".

Since testing for the case when no epoch argument is passed might be more important than handling warnings, it might make sense to just suppress the warning if no better solution can be reached.

  • The reason for ignoring the dubious year warnings, again from the PR description might be a good choice since testing for long propagations might be important.

Question
Since Orbit.pqw() is now deprecated and is going to be removed (as per the warning message) and that tests for it are also removed, would it be fine to remove Orbit.pqw completely? I think this might stabilize the coverage because currently the tests for pqw are removed but it is still in the source code. Just wanted to clarify this before making any changes :)

Thanks!

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 3, 2021

@astrojuanlu Could you please guide me on the above and let me know your thoughts?

Thanks!

@astrojuanlu
Copy link
Member

Hello @Yash-10! I'm about to review this PR. Would you please rebase on top of main?

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.

Did a thorough review, thanks for your patience!

src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
tests/tests_plotting/test_misc.py Outdated Show resolved Hide resolved
tests/tests_twobody/test_orbit.py Show resolved Hide resolved
tests/tests_twobody/test_orbit.py Show resolved Hide resolved
tests/tests_twobody/test_orbit.py Show resolved Hide resolved
tests/tests_twobody/test_orbit.py Show resolved Hide resolved
tests/tests_twobody/test_propagation.py Show resolved Hide resolved
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 go! My only remaining question is: did we have to regenerate the images? The changes look like a 1 pixel displacement, and a diff on the tens or hundreds of bytes. Were image tests failing locally because of it? If not, I think we should remove the changes and squash the commit, to avoid increasing the size of the repository.

@astrojuanlu
Copy link
Member

Merging & squashing after the tests pass 👍🏽

@astrojuanlu astrojuanlu enabled auto-merge (squash) June 11, 2021 08:53
@astrojuanlu
Copy link
Member

(Not sure the automerge will work because of the coverage check, but let's see)

@astrojuanlu astrojuanlu merged commit b37e5fe into poliastro:main Jun 11, 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.

None yet

3 participants