Emicorr speedup #9077
Emicorr speedup #9077
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9077 +/- ##
==========================================
+ Coverage 78.14% 78.35% +0.20%
==========================================
Files 505 505
Lines 46308 46057 -251
==========================================
- Hits 36188 36087 -101
+ Misses 10120 9970 -150
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Regression tests were run here: likely due to astropy relying on bottleneck and no longer emitting a warning for all nan input. |
|
Tested (after removal of the bottleneck code) with jw01283001001_03101_00001_mirimage_uncal.fits (full frame, 10 ints, 100 groups), jw01668002001_04101_00001_mirimage_uncal.fits (4qpm subarray, 6 ints, 1251 groups), and jw04496004001_03103_00001-seg001_mirimage_uncal.fits (slitless subarray, 288 ints, 10 groups). Runtime goes from 127 to 80 seconds, 127 to 42 seconds, and 12 to 9 seconds respectively. |
| @@ -282,30 +280,32 @@ def apply_emicorr(output_model, emicorr_model, | |||
|
|
|||
| for ninti in range(nints): | |||
| log.debug(' Working on integration: {}'.format(ninti+1)) | |||
|
|
|||
| # Read in this integration | |||
| data = output_model.data[ninti].copy() | |||
There was a problem hiding this comment.
Is there a reason this copy improves performance/memory consumption? It seems weird to me that you couldn't just be set as data = data[ninti].
There was a problem hiding this comment.
@kmacdonald-stsci there are three reasons for the improvement:
- The copy of data is done inside a loop, one integration at a time, rather than copying the entire array outside the loop. This array is being modified inside the loop and I have not checked in detail to see if it is safe not to copy; it was much easier to just copy one slice at a time rather than check to see if the copy is absolutely necessary.
np.whereis expensive! For example,np.where(x == 2)first evaluatesx == 2to create a boolean array, and thennumpyconstructs oneint64array for each dimension ofx. As another example,
badpix = np.where(~np.isfinite(x))
x[badpix] = 0
will likely be much slower than
x[~np.isfinite(x)] = 0
or, equivalently, if you want to be explicit,
badpix = ~np.isfinite(x)
x[badpix] = 0
The latter two options never createint64arrays.- Finally, array slicing is much more efficient than advanced indexing, which requires a more general loop and array copy under the hood. Example:
np.mean(x[1::4])
is much faster than
np.mean(x[np.arange(1, len(x), 4)])
Point (1) here is why memory usage is less; points (2) and (3) are why it runs faster.
|
Running regression tests here: |
Resolves JP-3819
Closes #
This PR significantly improves the speed and memory usage of emicorr with very small changes: moving an array copy inside a loop, using native array slicing rather than indexing with integer arrays, and (optionally) using bottleneck's implementations of nanmedian. The user is warned if bottleneck is not installed. For jw01668002001_04101_00001_mirimage on my Mac, the EMI step goes from 120s to 30s with bottleneck installed.
Tasks
Build 11.3(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see below for change types)docs/pageokify_regteststo update the truth filesnews fragment change types...
changes/<PR#>.general.rst: infrastructure or miscellaneous changechanges/<PR#>.docs.rstchanges/<PR#>.stpipe.rstchanges/<PR#>.datamodels.rstchanges/<PR#>.scripts.rstchanges/<PR#>.fits_generator.rstchanges/<PR#>.set_telescope_pointing.rstchanges/<PR#>.pipeline.rststage 1
changes/<PR#>.group_scale.rstchanges/<PR#>.dq_init.rstchanges/<PR#>.emicorr.rstchanges/<PR#>.saturation.rstchanges/<PR#>.ipc.rstchanges/<PR#>.firstframe.rstchanges/<PR#>.lastframe.rstchanges/<PR#>.reset.rstchanges/<PR#>.superbias.rstchanges/<PR#>.refpix.rstchanges/<PR#>.linearity.rstchanges/<PR#>.rscd.rstchanges/<PR#>.persistence.rstchanges/<PR#>.dark_current.rstchanges/<PR#>.charge_migration.rstchanges/<PR#>.jump.rstchanges/<PR#>.clean_flicker_noise.rstchanges/<PR#>.ramp_fitting.rstchanges/<PR#>.gain_scale.rststage 2
changes/<PR#>.assign_wcs.rstchanges/<PR#>.badpix_selfcal.rstchanges/<PR#>.msaflagopen.rstchanges/<PR#>.nsclean.rstchanges/<PR#>.imprint.rstchanges/<PR#>.background.rstchanges/<PR#>.extract_2d.rstchanges/<PR#>.master_background.rstchanges/<PR#>.wavecorr.rstchanges/<PR#>.srctype.rstchanges/<PR#>.straylight.rstchanges/<PR#>.wfss_contam.rstchanges/<PR#>.flatfield.rstchanges/<PR#>.fringe.rstchanges/<PR#>.pathloss.rstchanges/<PR#>.barshadow.rstchanges/<PR#>.photom.rstchanges/<PR#>.pixel_replace.rstchanges/<PR#>.resample_spec.rstchanges/<PR#>.residual_fringe.rstchanges/<PR#>.cube_build.rstchanges/<PR#>.extract_1d.rstchanges/<PR#>.resample.rststage 3
changes/<PR#>.assign_mtwcs.rstchanges/<PR#>.mrs_imatch.rstchanges/<PR#>.tweakreg.rstchanges/<PR#>.skymatch.rstchanges/<PR#>.exp_to_source.rstchanges/<PR#>.outlier_detection.rstchanges/<PR#>.tso_photometry.rstchanges/<PR#>.stack_refs.rstchanges/<PR#>.align_refs.rstchanges/<PR#>.klip.rstchanges/<PR#>.spectral_leak.rstchanges/<PR#>.source_catalog.rstchanges/<PR#>.combine_1d.rstchanges/<PR#>.ami.rstother
changes/<PR#>.wfs_combine.rstchanges/<PR#>.white_light.rstchanges/<PR#>.cube_skymatch.rstchanges/<PR#>.engdb_tools.rstchanges/<PR#>.guider_cds.rst