JP-1796: Fix wfs_combine bugs; add unit tests #5519
Conversation
| err_data_1 = im_1_a.err.copy() | ||
| err_data_2 = im_2_a.err.copy() | ||
|
|
||
| bad1 = np.bitwise_and(dq_data_1, dqflags.pixel['DO_NOT_USE']) |
There was a problem hiding this comment.
The bug was here (and 2 lines down). np.bitwise_and returns an array of 0 and 1 values, and cannot be used to index another array without being converted to a boolean array. Once converted, it can have the True/False values flipped via that ~ operator.
An example:
In [1]: import numpy as np
In [2]: a = np.arange(15)
In [3]: a
Out[3]: array([ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 1
8000
4])
In [4]: np.bitwise_and(a, 1)
Out[4]: array([0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0])
Codecov Report
@@ Coverage Diff @@
## master #5519 +/- ##
==========================================
+ Coverage 71.25% 71.57% +0.32%
==========================================
Files 410 410
Lines 36321 37180 +859
Branches 5594 5763 +169
==========================================
+ Hits 25879 26613 +734
- Misses 8800 8913 +113
- Partials 1642 1654 +12
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report at Codecov.
|
|
Ha I see my problem (bug). Hopefully I will not make that mistake again. |
|
|
||
| # Save the output file | ||
| self.save_model( | ||
| output_model, suffix='wfscmb', output_file=outfile, format=False | ||
| ) | ||
|
|
||
| return None | ||
| return output_model |
There was a problem hiding this comment.
So lines 59-61 are still in there to save the results to a file, as well as now passing output_model back to stpipe. Is stpipe going to try to save the model to a file too?
There was a problem hiding this comment.
That's a good question. Let me check in the regression test run.
There was a problem hiding this comment.
Yes, it does auto write out each result under the wrong name. Lovely. Let me fix this.
There was a problem hiding this comment.
So if I put it into a if self.save_results: block, it will then save it out properly. One then needs to supress the autosaving because there's the return statement. Perhaps @stscieisenhamer has a better way of handling this. I think this can be taken care of in issue #5511, as currently this step theoretically could return multiple products, but has no way to do so without utilizing a ModelContainer.
There was a problem hiding this comment.
What kind of wrong name is stpipe using when it saves to file? We should be able to tell it the proper product name suffix. @stscieisenhamer is the expert in that.
There was a problem hiding this comment.
And one also needs control over the format arg in Step.save_model(format=False). Without this, one gets:
jw00632-o003_t001_nircam_f212n-wlp8-nrca4_{suffix}-05_wfscmb.fits
There was a problem hiding this comment.
The formatting template in the association file is:
"name": "jw00632-o003_t001_nircam_f212n-wlp8-nrca4_{suffix}-05",
There was a problem hiding this comment.
So the current state of this PR produces the correct name, doesn't produce the incorrect name, and does so by hijacking the knowledge of when stpipe turns on save_results. This is not ideal, but given that doing it properly would require a small refactor, I don't think we can do it before 7.7
There was a problem hiding this comment.
I think the only time this step would return multiple results is if you gave it an input ASN file that defines multiple products. This is not the case in normal operations (only 1 product per ASN).
There was a problem hiding this comment.
I added some comments to why it is implemented in this hacky way now. In the future, perhaps we can refactor Step.save_model to expose format as a class or instance attribute.
Once tests pass I will merge.
Count on making it again. 😅 I've made this mistake countless times myself, and did while trying to debug this. Writing the test sorted me out. |
Fixes some bugs introduced in #5500 that showed up in the
wfs_combineregression tests. Adds some unit tests for the expected behavior.Resolves #5515 / JP-1796