Removed redundant calculations, unneeded array allocations from jump step #302
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
==========================================
- Coverage 86.38% 86.36% -0.02%
==========================================
Files 49 49
Lines 8899 8892 -7
==========================================
- Hits 7687 7680 -7
Misses 1212 1212 ☔ View full report in Codecov by Sentry. |
|
The romancal tests for these changes pass, |
|
JWST regtests are running here: Failures are unrelated (from spacetelescope/jwst#8893). |
There was a problem hiding this comment.
These changes look pretty straightforward, and seem worth merging as is, before we do any more profiling or refactoring for jump.
One quick question about the change to the flag handling, below. Also, the change note needs to be moved out of the CHANGES.rst file to a newer style towncrier note -- see the PR template description for the new method. That should fix the readthedocs error and the changelog check.
| dat[np.where(np.bitwise_and(gdq, sat_flag))] = np.nan | ||
| dat[np.where(np.bitwise_and(gdq, dnu_flag))] = np.nan | ||
| dat[np.where(np.bitwise_and(gdq, dnu_flag + sat_flag))] = np.nan |
There was a problem hiding this comment.
Is there any chance that either the dnu or sat flags might be set without both being set?
There was a problem hiding this comment.
The bottom line subsumes the previous two, right? As long as dnu_flag and sat_flag are different, bitwise_and will be nonzero any time either of those bits is set, so np.where will pick out all points where either flag is set? I tested this myself to be absolutely sure.
There was a problem hiding this comment.
In general, the syntax
np.where(np.bitwise_and(gdq, flag_a + flag_b + flag_c))
should return all pixels where at least one of flags a, b, and c is set. If it is clearer, we could write something like
dat[np.bitwise_and(gdq, flag_a + flag_b + flag_c) != 0] = np.nan
instead.
There was a problem hiding this comment.
Oh yes, you're right. I always struggle with bit-wise math. Thanks!
There was a problem hiding this comment.
Sure! The only hole this opens is if someone is foolish enough to make sat_flag and dnu_flag equal. Hopefully that gets checked somewhere? :-)
There was a problem hiding this comment.
Pretty sure that's not possible, but to be super safe, we could cover that case by using | instead of +.
|
|
||
| - [jump] Remove redundant calculations and unneeded array allocations. | ||
| (`JP-3697<https://jira.stsci.edu/browse/JP-3697>`_) |
There was a problem hiding this comment.
This needs to be removed and added to a towncrier change note instead.
|
@t-brandt - just checking in. I think this PR just needs some minor updates and it should be ready to go. Do you want to make the changes, or would you like me to do it for you? |
Resolves JP-3697
Closes #
This PR affects the two-point-difference jump step only. It removes duplicate and redundant calculations and unneeded array allocations, which make a significant difference for large uncal files. Masked array median is also changed to nanmedian, and a few variables are renamed to avoid overwriting a quantity that is needed at the end of the routine.
Tasks
docs/pageno-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)"git+https://github.com/<fork>/stcal@<branch>")jwstregression testromancalregression testnews fragment change types...
changes/<PR#>.apichange.rst: change to public APIchanges/<PR#>.bugfix.rst: fixes an issuechanges/<PR#>.general.rst: infrastructure or miscellaneous change