-
Notifications
You must be signed in to change notification settings - Fork 26
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
Change warning type for some kernels. Add unit tests for flux conservation #141
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
=======================================
Coverage 75.29% 75.29%
=======================================
Files 7 7
Lines 344 344
=======================================
Hits 259 259
Misses 85 85 ☔ View full report in Codecov by Sentry. |
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.
Looks good to me.
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.
What happens with the tophat
kernel now? I lost track.
drizzle/tests/test_drizzle.py
Outdated
) | ||
|
||
@pytest.mark.parametrize( | ||
'kernel,conserves', |
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.
Is conserves
used anywhere? Am I missing it?
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.
good observation. Initially, I was thinking of a different way to design the test but then decided to go with pytest.mark.xfail
for non-conserving kernels. Fixed in the next commit
drizzle/tests/test_drizzle.py
Outdated
), | ||
], | ||
) | ||
def test_flux_conservation_distorted(kernel, conserves): |
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.
Same question here
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.
fixed
Nothing for now. It was deprecated in #140. This PR simply adds unit tests to test flux (non-)conservation. |
This PR adds unit tests that verify that "square", "turbo", and "point" kernels do conserve flux.
It also modifies the warning type for the "non flux conserving" kernels from
DeprecationWarning
toWarning
.