JP-3997: Enable interpolation in pixel map creation for imaging data in resample#10137
JP-3997: Enable interpolation in pixel map creation for imaging data in resample#10137emolter wants to merge 18 commits intospacetelescope:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10137 +/- ##
==========================================
- Coverage 85.98% 85.33% -0.65%
==========================================
Files 368 370 +2
Lines 38457 39132 +675
==========================================
+ Hits 33066 33393 +327
- Misses 5391 5739 +348 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
initial regtest round https://github.com/spacetelescope/RegressionTests/actions/runs/21000948898 The five failures all look very small.
|
|
new round of regtests after merging and okifying the stcal PR: https://github.com/spacetelescope/RegressionTests/actions/runs/21924519427 |
|
Based off #9440 (comment) shouldn't these be optional step parameters that INS can test and then chose to enable via pars files? |
|
As I stated at spacetelescope/romancal#2136 (comment) I'm not particularly happy with doing it that way. I had not realized that David had specifically requested this though, and I guess what he and @tapastro say goes. |
mcara
left a comment
There was a problem hiding this comment.
I would also suggest updating stepsize and order to pixmap_stepsize and pixmap_order to keep this in agreement with changes in stcal and romancal.
jwst/resample/resample_spec.py
Outdated
| stepsize=1, | ||
| order=1, |
There was a problem hiding this comment.
Why is this hardcoded to use the full (not "sparse") pixel map?
There was a problem hiding this comment.
same reason as above re spectroscopic data, although if we do indeed make all the parameters available at the step level then I suppose users will be able to decide if they want to turn it on
jwst/outlier_detection/utils.py
Outdated
| stepsize=1, | ||
| order=1, |
There was a problem hiding this comment.
Why is this hardcoded to stepsize=1?
There was a problem hiding this comment.
This method is only used for outlier detection of spectroscopic data. In talking with Tim and others earlier we had initially decided not to attempt this for spectroscopic data. Has that changed? If so, I'd be happy to modify this.
There was a problem hiding this comment.
I still think we probably shouldn't attempt the speed up for resampling spectral data without further study.
There was a problem hiding this comment.
I thought it was disabled because when testing it with the original version of the stcal PR that did not account for the bounding boxes of spectral images it resulted in all NaN pixel maps - see my comment in spacetelescope/stcal#488 (comment) This, however, has been fixed.
|
https://github.com/spacetelescope/RegressionTests/actions/runs/21959536402 All passing as expected, since this is not on by default |
|
The last few commits should have addressed comments from @mcara and @braingram. Talking with @tapastro and @drlaw1558 , we are indeed going to make these parameters available at the step level for imaging data, and request for the INS teams to test them out for a build or two before turning this on by default. |
jwst/resample/resample.py
Outdated
| Interpolation is only performed if ``pixmap_stepsize > 1``. Default is 1. | ||
|
|
||
| pixmap_order : int, optional | ||
| Order of the pixel map interpolation used. Default is 1. |
There was a problem hiding this comment.
| Order of the pixel map interpolation used. Default is 1. | |
| Interpolating spline order for pixel map computation. Default is 1. |
jwst/resample/resample.py
Outdated
| pixmap_stepsize : int, optional | ||
| Step size for pixel map interpolation during resampling. | ||
| Larger step sizes result in faster performance at the cost of accuracy. | ||
| Interpolation is only performed if ``pixmap_stepsize > 1``. Default is 1. |
There was a problem hiding this comment.
stepsize is not required to be integer.
| pixmap_stepsize : int, optional | |
| Step size for pixel map interpolation during resampling. | |
| Larger step sizes result in faster performance at the cost of accuracy. | |
| Interpolation is only performed if ``pixmap_stepsize > 1``. Default is 1. | |
| pixmap_stepsize : float, optional | |
| Indicates the spacing at which WCS is evaluated when computing pixel map. WCS coordinates of the full pixel map is computed by interpolating over this sparse pixel map when ``pixmap_stepsize > 1``. | |
| Larger step sizes result in faster performance at the cost of accuracy. Default is 1. |
jwst/resample/resample_step.py
Outdated
| enable_err = boolean(default=True) # Compute and report the err array | ||
| report_var = boolean(default=True) # Report the variance array | ||
| pixmap_stepsize = integer(default=1) # Interpolation step size for pixel map; interpolation is used for stepsize > 1 | ||
| pixmap_order = integer(default=1) # Order of the pixel mapping polynomial fit, must be 1 or 3 |
There was a problem hiding this comment.
| pixmap_order = integer(default=1) # Order of the pixel mapping polynomial fit, must be 1 or 3 | |
| pixmap_order = integer(default=1) # Spline order for pixel mapping, must be 1 or 3 |
First, it is not a "fit" - it is interpolation. Second, "polynomial" is too general, for example piece-wise polynomial interpolation is generally different from spline (which guaranties interpolating function smoothness when order>1).
changes/10137.resample.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Add ``pixmap_stepsize`` and ``pixmap_order`` parameters to allow interpolated calculation of pixel map for imaging mosaics; improves runtime of calwebb_image3 by 20 percent for a 30-image mosaic tested, with bigger improvements expected for larger mosaics. | |||
There was a problem hiding this comment.
with bigger improvements expected for larger mosaics.
I’m not fully convinced that larger output mosaics will translate into additional speedups. Since WCS evaluation cost mainly depends on the size of the input images, the practical improvement is still limited to about stepsize**2 (minus spline overhead) - and pixel map computation is only a fraction of the total resample step. Unless JWST cal images become larger, we may not see further gains. It might be helpful for each instrument team to identify the largest stepsize that preserves scientifically valid distortion behavior and include those recommendations in the calibration reference files.
There was a problem hiding this comment.
That's fair, I think it's probably sufficient to just quote the single test I did here, without additional commentary
changes/10137.outlier_detection.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Add pixmap_stepsize and pixmap_order parameters to allow interpolated calculation of pixel map for imaging mosaics when resampling | |||
There was a problem hiding this comment.
| Add pixmap_stepsize and pixmap_order parameters to allow interpolated calculation of pixel map for imaging mosaics when resampling | |
| Add ``pixmap_stepsize`` and ``pixmap_order`` parameters to allow interpolated calculation of pixel map for imaging mosaics when resampling |
Resolves JP-3997
Closes #9440
This PR updates calls to
gwcs_blotduring resampling of imaging data to take advantage of interpolated computations made available by spacetelescope/stcal#488. Spectroscopic data are not affected.I have confirmed that this change meaningfully improves runtime.
For a tiny (3-image) subset of the large mosaic program jw04111, the time spent in
calc_pixmapwent from 6.6 seconds on main to 0.8 seconds on the PR branch. In this case,calc_pixmapwas not the rate-limiting step - overall runtime of calwebb_image3 went from 120s to 113s, a 5% decrease.For a larger (~30-image) subset of the same program, the time spent in
calc_pixmapwent from 123 seconds on main to 14 seconds on the PR branch. The fractional runtime gain of calwebb_image3 was more substantial, going from 530s to 430s, a 20% decrease.I'd anticipate that the fractional gain here will become larger for bigger mosaics, although I don't really feel like waiting for the full many-hundreds-of-images dataset to run through.
Tasks
Build 12.0(use the latest build if not sure)no-changelog-entry-needed)changes/:echo "changed something" > changes/<PR#>.<changetype>.rst(see changelog readme for instructions)changes/<PR#>.breaking.rstnews fragmentdocs/pageokify_regteststo update the truth files