-
Notifications
You must be signed in to change notification settings - Fork 169
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
JP-3470: Add ifu covariance scaling to extract1d #8457
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8457 +/- ##
==========================================
+ Coverage 57.93% 57.98% +0.05%
==========================================
Files 387 387
Lines 38839 38826 -13
==========================================
+ Hits 22502 22514 +12
+ Misses 16337 16312 -25 ☔ 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 like a straight forward addition.
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. Just one comment on the updated documentation.
regtest run started at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1424 |
CI tests are failing with:
|
Hm, I thought the default argument of 1.0 in the function call would be enough to deal with this, but I guess not. Added a check for value defined prior to doing anything with it. |
I'd like to check the issue with the parameter default value not being picked up before we merge this, to verify it's not a larger issue. |
Started another regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1454 And the CI tests seem to suddenly be working OK. Not sure why ... |
Latest regtest run is clean. Only failures are unrelated (ref file updates). So this is ready to merge as soon as @nden is OK with the earlier issue with function arguments. |
I tracked the problem with the arguments to a test So passing of parameters works as expected but if extract.do_extract_1d is to be called on its own I suggest we set a default value in the signature as well. Also remove the condition here. |
Alternatively, instead of removing the condition here, it could be modified to only trigger when |
I've changed that None default to 1.0, and changed the conditional to trigger when the value is not 1.0 |
Final updates look good to me. Merging. |
Resolves JP-3470 by allowing for a parameter to be passed into extract1d accounting for IFU cube covariance.
Closes #8083
Checklist for maintainers
CHANGES.rst
within the relevant release sectionHow to run regression tests on a PR