Skip to content
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-3664: Fix pixel replace numpy 2.0 issues #9004

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

drlaw1558
Copy link
Collaborator

@drlaw1558 drlaw1558 commented Dec 11, 2024

This PR addresses discrepancies found in the MIRI LRS pixel replacement step when using numpy 2.0; these trace back to failures in the scipy.minimize 'BFGS' algorithm that result in failed minimizations, resulting in the incorrect scaling applied to replaced pixels when using the fit_profile pixel replacement method. A trivial fix is to use the Nelder-Mead algorithm instead of the default BFGS, which does not have the same numpy 2.0 issues.

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
  • changes/<PR#>.general.rst: infrastructure or miscellaneous change
  • changes/<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

@drlaw1558 drlaw1558 requested a review from a team as a code owner December 11, 2024 22:47
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@4bd98bb). Learn more about missing BASE report.
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
jwst/pixel_replace/pixel_replace.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9004   +/-   ##
=======================================
  Coverage        ?   66.26%           
=======================================
  Files           ?      375           
  Lines           ?    37954           
  Branches        ?        0           
=======================================
  Hits            ?    25151           
  Misses          ?    12803           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drlaw1558 drlaw1558 requested a review from tapastro December 12, 2024 04:56
@melanieclarke melanieclarke added this to the Build 11.2 milestone Dec 12, 2024
Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me. Running the MIRI LRS spec2 regtest locally, I see some minor differences between the pixel replace values on main and on this branch, and between numpy 1 and 2 on this branch, but nothing significant. I will run the full regtests in a moment.

@melanieclarke
Copy link
Collaborator

@melanieclarke
Copy link
Collaborator

Comparing the regression test outputs for jwst/regtest/test_nirspec_fs_spec2.py::test_nirspec_fs_spec2_pixel_replace[jw013090_prtest_04102_00004_nrs2_pixel_replace.fits] (with numpy 1), it looks to me like this change introduces more bad pixel replacements than the old version.

See, for example x=842, y=24 in jw013090_prtest_04102_00004_nrs2_pixel_replace.fits - replaced value in this branch is -22.88, replaced value on main is a more reasonable 1.76.

@drlaw1558
Copy link
Collaborator Author

@melanieclarke : What file does that correspond to? I'm not sure what 'prtest' in the filename is drawn from.

@melanieclarke
Copy link
Collaborator

@melanieclarke : What file does that correspond to? I'm not sure what 'prtest' in the filename is drawn from.

It's named a little oddly to avoid a conflict with output products from a different test, run without pixel replace. The input file is jw01309022001_04102_00004_nrs2_rate.fits. I can point you to the output products from the regression test on artifactory if that's helpful.

@drlaw1558
Copy link
Collaborator Author

Hm, I'm having problems reproducing the issue offline with the jw01309022001_04102_00004_nrs2_rate.fits file (which contains multiple SCI extensions, but I assume the relevant one is the one with an obvious source). If you can point me at the artifactory input/output data that would be useful.

@drlaw1558
Copy link
Collaborator Author

Looks like my cal file differed substantially from the one in artifactory; using that as input I can reproduce the issue, looks like some different behavior by the minimization routine depending on the algorithm in use. In this case I think the fundamental issue may be that the code is designed (I think) around the assumption that the brightest GOOD pixel in a given cross-dispersion cut is due to a source. That seems to be failing in the NIRSpec slit case, when there can be very faint/bright pixels at the edge of the slit that should really be getting masked out by the flatfield but aren't.

Will have to think a bit more about what the solution to that is.

@melanieclarke
Copy link
Collaborator

Thanks for testing. It sounds like the increased bad fits are actually appropriately bad, then? As in, it's not the fitting routine's fault, it's the input data? In that case, we can probably go ahead and make the change to the algorithm, since it has clear advantages in numpy 2, and work on getting it better input data later.

@drlaw1558
Copy link
Collaborator Author

I've introduced a few more changes that should hopefully fix the issue. This makes the routine itself a little more resilient, so that it isn't trying to scale profile fits on noise when there's no signal in the data. Let's see how this version works with the reg tests.

@melanieclarke
Copy link
Collaborator

melanieclarke commented Dec 13, 2024

Thanks, those changes look promising. I'll update from main and run again.

Running again here:
https://github.com/spacetelescope/RegressionTests/actions/runs/12319915643

@drlaw1558
Copy link
Collaborator Author

As an aside though, @hayescr - there are some of the NIRSpec slits reporting CAL values of +- 1e22 on the edges of the slit (in empty fields) that we probably want to look into the flatfield correction for.

@melanieclarke
Copy link
Collaborator

melanieclarke commented Dec 13, 2024

Testing locally on jw01309022001_04102_00004_nrs2_cal.fits, some of the replacements in the first science extension, where there is no source, look better with this change, but there are some concerning new artifacts in the second science extension, for which there is a source.

Top is with the Nelder-Mead algorithm, no SNR checks; bottom is Nelder-Mead with SNR checks.

jw01309022001_04102_00004_nrs2_pixel_replace

@hayescr
Copy link
Contributor

hayescr commented Dec 13, 2024

As an aside though, @hayescr - there are some of the NIRSpec slits reporting CAL values of +- 1e22 on the edges of the slit (in empty fields) that we probably want to look into the flatfield correction for.

Thanks @drlaw1558 I can take a look and pass this on. Do you happen to know if the slit in question was S200B1 (that's the only slit in jw01309022001_04102_00004_nrs2_cal that has values in the 1e22 range)? If so that slit is not fully calibrated, since it is a backup/redundant slit and is not recommended for science, so I think that is expected at the moment.

@melanieclarke melanieclarke modified the milestones: Build 11.2, Build 11.3 Dec 13, 2024
@drlaw1558
Copy link
Collaborator Author

One more version... This time dealing with the unique case where for some source types NIRSpec has science units around 1e-11. @hayescr - yes, the problem slit with 1e22 values looks like S200B1. Good to know that we can expect weird things in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants