-
Notifications
You must be signed in to change notification settings - Fork 34
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
Provide correct slope for color ratios with SpectralGradient.from_color
#392
Provide correct slope for color ratios with SpectralGradient.from_color
#392
Conversation
To be done: rebase after #383 is merged. Update change log. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #392 +/- ##
=======================================
Coverage 82.99% 82.99%
=======================================
Files 88 88
Lines 8108 8109 +1
=======================================
+ Hits 6729 6730 +1
Misses 1379 1379 ☔ View full report in Codecov by Sentry. |
5f21661
to
ac9de58
Compare
Hello @mkelley! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-08-28 15:09:26 UTC |
Sorry, just getting to this now. In the code diff view, Codecov indicates that there are several new lines not covered by tests. Is this a problem (I note that the Codecov summary report on this page says that all "coverable" lines are covered)? Otherwise, the changes look good to me. |
Those reported lines don't make sense. They show up as covered in the codecov app: https://app.codecov.io/gh/NASA-Planetary-Science/sbpy/pull/392/blob/sbpy/spectroscopy/core.py |
c1c660a
to
186db1b
Compare
I've rebased. Maybe that will fix them? If not, I think the coverage report should take precedence. |
u.Magnitudes (e.g., from the difference of two VEGAmags) do not support the unary -. Instead mulitply by -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that sounds fine to me. Yeah, the contradiction between the different reports is a bit strange (I still see the comments in the code diffs).
I can refresh the codecov check and it'll report 100% covered as it should. But the diff report always lists those lines as uncovered. Anyway, something to keep in mind in other reviews. |
Addresses #389