-
Notifications
You must be signed in to change notification settings - Fork 170
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-3597: Add new algorithm for EMI fit; clean up existing code #9216
base: main
Are you sure you want to change the base?
Conversation
Initial regression tests here: Regtests show no diffs, as expected. |
@t-brandt - here is the new PR for the emicorr updates. Please review and let me know how it looks to you. I'll hold off on collecting other reviews until you're happy with it. I would especially like your eyes on the documentation - please let me know if I have something wrong or if there's more you think should be added. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9216 +/- ##
==========================================
+ Coverage 73.65% 73.95% +0.29%
==========================================
Files 368 368
Lines 36374 36521 +147
==========================================
+ Hits 26792 27009 +217
+ Misses 9582 9512 -70 ☔ View full report in Codecov by Sentry. |
Thanks @melanieclarke ! A few quick comments:
|
Done! The documentation comment is now cleaned up as well. |
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.
This looks great, @melanieclarke, thank you! I added some very minor comments. The only one that may require a little thought and attention is the criterion used to determine which pixels are appropriate to fit the EMI amplitude and phase.
jwst/emicorr/emicorr.py
Outdated
# "Good" pixel here has no more than twice the median standard | ||
# deviation among group differences and is not flagged in the pdq | ||
# array. This should discard most bad and high-flux pixels. | ||
|
||
pixel_std = np.std(data, axis=1) | ||
pixel_ok = (pixel_std < 2 * np.median(pixel_std)) & (pdq == 0) |
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.
My comment here is slightly incorrect: I am using pixels with no more than twice the median standard deviation among group values. We either need to change the comment or change the following line to
pixel_std = np.std(np.diff(data, axis=1), axis=1)
I don't think it will matter much for performance--I tried this out and had trouble telling the difference.
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 will change the comment for now. Let me know if you think of a reason to change the code instead.
56a1fd0
to
219eb28
Compare
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.
Thanks for this work, Melanie! Looks great! just a couple of very minor things.
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.
LGTM!
5244377
to
a2aaa50
Compare
a2aaa50
to
6aef2c9
Compare
Resolves JP-3597
Resolves JP-3674
Add a new algorithm for fitting and removing EMI noise in MIRI ramps.
In addition, a few minor bugs in the existing algorithm are fixed here:
minmed
function was not performing minimum calculations instead of median when nframes <= 2 as describedI have also added almost-complete unit test coverage for the new and existing algorithms, fixed code style for the new standards, updated documentation, and added a regression test for the new algorithm. Truth files for the new regression test were generated locally with this branch, so I do not expect diffs.
This PR replaces #9187, which implemented the new algorithm only. Changes introduced in that PR are all included here, with these further edits:
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 filesnews 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#>.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