-
Notifications
You must be signed in to change notification settings - Fork 10
add unit, regression, edge case testing #126
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 47.08% 39.50% -7.59%
==========================================
Files 106 107 +1
Lines 6872 6971 +99
==========================================
- Hits 3236 2754 -482
- Misses 3636 4217 +581 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| photfnu = sample_throughput.photfnu | ||
|
|
||
| assert photfnu is not None, "PHOTFNU must be computable" | ||
| assert photfnu > 0, f"PHOTFNU must be positive, got {photfnu}" |
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.
There are some checks like this throughout that I think should be an allclose on the exact value to make sure the calculated value isn't changing, since this should be a deterministic calculation. As it it isn't useless, but it could easily be more useful. Similarly, there are various places that are checking that the length of arrays is > 0 where we could be checking the actual values and/or exact length of the array.
| sed = su.photometry.SED(wave, flux) | ||
| sed.normalize(10000., 5.0) | ||
| assert np.isclose(sed['flam'][50], 5.0, rtol=0.1) | ||
| assert np.allclose(sed['flam'][50], 5.0, rtol=0.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.
Sorry, I didn't mean to imply that isclose needed to change to allclose, I'm just in the habit of using allclose since I'm usually working with arrays. Probably don't need to change it back though 🤷
Added 63 new unit tests across 4 test modules to improve coverage of core slitlessutils functionality:
test_wfss_simulated.py
test_sources.py
test_photometry.py
test_config.py