-
Notifications
You must be signed in to change notification settings - Fork 167
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-3757: Use squared drizzle coeffs for variance and error arrays #8997
base: main
Are you sure you want to change the base?
Conversation
fc2e46c
to
812946a
Compare
Hi Mihai, FYI, I'm at a conference this week and so won't be prompt to review here. But more generally, what part of this PR are you most interested in having reviewed? This PR, or something more like this bit on drizzle? |
That is exactly the part that I was thinking about for you in this PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8997 +/- ##
==========================================
+ Coverage 64.56% 66.60% +2.03%
==========================================
Files 375 375
Lines 38890 37898 -992
==========================================
+ Hits 25109 25241 +132
+ Misses 13781 12657 -1124 ☔ 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.
I like these changes a lot! Thank you for working on this!
I think it would be a good idea to add some unit tests for the output error values when a pixel scale is applied - it looks like currently only the data values are tested in that case.
Documentation will need to be updated, in docs/jwst/resample/main.rst -- there is a note there about error propagation that will need replacing.
Thanks @mcara ! I'm seeing some very strange results though in testing; here's the VAR_RNOISE extension from b11.1 (left) and the revised drizzle (right) for resampling a single MIRI input frame. Perhaps related to @melanieclarke 's comments on pixel scale? |
@drlaw1558 I have addressed @melanieclarke comments about pixel scale last night. So if your tests are recent, this should not be an issue. |
the image on the right is gorgeous though (or fascinating) |
@mcara Indeed! I've confirmed the structure in a hacked input with uniform variances, and that's even more impressive. This is, however, with what should be the latest version (as of 30 minutes ago) of both your jwst and drizzle branches. My best guess without digging into the code more closely yet is something to do with the input pixel scale as it shows up even when the input variance values are completely uniform. |
May I ask what is the variation in the resampled (variance) image when input variance is constant? |
In this case I set VAR_POISSON=0.013 everywhere in the input cal file, and the output VAR_POISSON values are varying from about 0.003 to 0.013 |
The scale that @melanieclarke refers to is a constant factor common to all pixels. It cannot be responsible for the observed pattern. So I investigated this to understand what is causing it. I did this investigation using a MIRI image that I had available locally. Please refer to equations (4) and (5) in Fruchter and Hook paper on Drizzle. For simplicity, let's ignore total weight
where Using propagation of errors and assuming independent uncertainties for input pixels, we get:
Assuming a constant variance for input pixels (just like you did in your simulation, @drlaw1558 ), the variance of a resampled (output) pixel is proportional to a sum of areas of overlap squared. When I did this for all output pixels (for my MIRI test image), I got this image: Basically, this is caused by contributions from varying fraction of overlap between the output pixels and overlapping input pixels (weights). These weights are squared for variance computations (uncertainty propagation). To illustrate this, refer to Fig 2 in the above paper. I modified it to label overlaps from different input pixels: So, as I mentioned,
So, you can see that |
I didn't anticipate the large scale pattern, but these values do make sense - we would expect the variance to be at most 0.013, for pixels with only one input pixel contributing, and less for pixels with up to 4 input pixels contributing. I'll test on some NIRSpec spectral data - I bet these patterns will align nicely with some residual resampling effects we see sometimes. |
I am still at a conference and haven't gotten a chance to dig in. But I'd mention:
|
bf553f5
to
6cf0b3a
Compare
db155c0
to
bf24e67
Compare
bf24e67
to
64aad44
Compare
3bd0f8e
to
b86e6ce
Compare
b86e6ce
to
7531f55
Compare
@drlaw1558 Thanks for validating the math. |
Can this PR now be formally approved? |
I'll review again for any last clean up details, and check out the regtest results. We should merge and release the drizzle changes first, though, and update the pin in pyproject.toml before this is merged. |
I think I'd like to understand the implications of this a little more before we merge it. |
@melanieclarke Regression tests are listed in the top message. One is running right now. |
@drlaw1558 - should we defer this change for the next build then? |
I think it's clear that the variance information provided by this change is indeed more correct; my concern is the covariance. I.e., most people tends to ignore the covariance between resampled pixels when doing photometry or other kinds of analyses. Given the larger than (in hindsight naively) anticipated difference with doing the mathematically correct thing for variances, it would be good to make sure that the improved variances don't interact in unexpected ways with the lack of covariance treatment when looking at the statistics derived for actual extracted sources. That's not too onerous a task, but nonetheless probably longer than the next few days. I'd thus be inclined to defer merging this until 11.3 so that we can study the implications more fully now that we have a working implementation. |
Okay, sounds good. I'll move it to the next build milestone then. |
I agree with these concerns, however, proposed changes are correct mathematically and the current variance computation is just wrong (and still correlated, just not visibly obvious). IMO, we should at least get better variance estimation at this time and think about covariance in a different PR. Resampled variance (and data) are known to be correlated. |
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.
I reviewed the last set of regression tests, and changes look appropriate to me. Error and variance values for resampled and downstream products are generally a little smaller. There are also some unrelated changes to NIRSpec spec3 products in the results, due to some outlier detection parameter updates, so we will need to run again before we merge this.
There are a couple irrelevant comments in the new unit test that can be removed, but otherwise this all looks good to me.
I officially approve, from my perspective, but I will mark this as "Request changes" to make sure this doesn't get merged before the drizzle update is complete and INS is comfortable with the change.
Ok, have had a chance to do some more simulations, this time introducing two point sources, one atop a ridge in the moire pattern, and one atop a trough. I.e., one for which the output grid is well matched to the input grid, and the other for which it's almost completely out of phase with the sampling. Inputs have poisson and read noise, and confirmed that the nominal noise in the Monte Carlo cal.fits files matches the actual noise in the cal files. Run these Monte Carlo tests through resampling and source_catalog steps and extract the photometry. With the current pipeline: All pretty close to correct, although some possible minor gains for Star 2 that the pipeline isn't recognizing. The reason we got into this effort was to fix the scaling when drizzling to large pixels. With large (0.2 arcsec) pixels in the current pipeline: Pipeline SNR estimates on both extracted point sources are off by ~ a factor of two because variance isn't propagated correctly. The new code fixes the issue when drizzling to large pixels. With large (0.2 arcsec) pixels in this PR: Some small disagreements, but much close than the existing code. The problem, however, is applying the new code to the default pixel scale. With regular pixels in this PR: I.e., the SNR of extracted sources is off by ~ a factor of 2 for the source where the output drizzle grid didn't closely match the input grid. The reason why is because, for output drizzled pixel scales close to the input scale, the mistake in variance propagation in the drizzle code was very nearly cancelled out by the mistake of ignoring the covariance. If we fix only the first one, we're going to see the problem of the second more clearly in our normal pipeline photometry. This will require some further discussion on best path forward. |
Have you talked to Stefano about this? He has pretty strong feelings about how the variances "should" be computed in order to get the right variances of sums over large-ish areas (i.e., when the covariances matter). I have not tried to work that out myself, unfortunately. |
Agreed Stefano would be good to include in this discussion; likely early in the new year. |
Resolves JP-3757
This changes the way variance and error arrays in the output (resampled) images are computed by resampling them directly using squared drizzle coefficients basically to enable standard error propagation.
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_regtests
to update the truth filesRegression test(s):
https://github.com/spacetelescope/RegressionTests/actions/runs/12239324062
https://github.com/spacetelescope/RegressionTests/actions/runs/12293444575 (after likely final revision of
drizzle
PR)https://github.com/spacetelescope/RegressionTests/actions/runs/12300631913 (post 7531f55)
news fragment change types...
changes/<PR#>.general.rst
: infrastructure or miscellaneous changechanges/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.datamodels.rst
changes/<PR#>.scripts.rst
changes/<PR#>.fits_generator.rst
changes/<PR#>.set_telescope_pointing.rst
changes/<PR#>.pipeline.rst
stage 1
changes/<PR#>.group_scale.rst
changes/<PR#>.dq_init.rst
changes/<PR#>.emicorr.rst
changes/<PR#>.saturation.rst
changes/<PR#>.ipc.rst
changes/<PR#>.firstframe.rst
changes/<PR#>.lastframe.rst
changes/<PR#>.reset.rst
changes/<PR#>.superbias.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.rscd.rst
changes/<PR#>.persistence.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.charge_migration.rst
changes/<PR#>.jump.rst
changes/<PR#>.clean_flicker_noise.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.gain_scale.rst
stage 2
changes/<PR#>.assign_wcs.rst
changes/<PR#>.badpix_selfcal.rst
changes/<PR#>.msaflagopen.rst
changes/<PR#>.nsclean.rst
changes/<PR#>.imprint.rst
changes/<PR#>.background.rst
changes/<PR#>.extract_2d.rst
changes/<PR#>.master_background.rst
changes/<PR#>.wavecorr.rst
changes/<PR#>.srctype.rst
changes/<PR#>.straylight.rst
changes/<PR#>.wfss_contam.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.fringe.rst
changes/<PR#>.pathloss.rst
changes/<PR#>.barshadow.rst
changes/<PR#>.photom.rst
changes/<PR#>.pixel_replace.rst
changes/<PR#>.resample_spec.rst
changes/<PR#>.residual_fringe.rst
changes/<PR#>.cube_build.rst
changes/<PR#>.extract_1d.rst
changes/<PR#>.resample.rst
stage 3
changes/<PR#>.assign_mtwcs.rst
changes/<PR#>.mrs_imatch.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.exp_to_source.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.tso_photometry.rst
changes/<PR#>.stack_refs.rst
changes/<PR#>.align_refs.rst
changes/<PR#>.klip.rst
changes/<PR#>.spectral_leak.rst
changes/<PR#>.source_catalog.rst
changes/<PR#>.combine_1d.rst
changes/<PR#>.ami.rst
other
changes/<PR#>.wfs_combine.rst
changes/<PR#>.white_light.rst
changes/<PR#>.cube_skymatch.rst
changes/<PR#>.engdb_tools.rst
changes/<PR#>.guider_cds.rst