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-3794: Add trace-based extraction option for NIRSpec #9022

Merged
merged 23 commits into from
Jan 9, 2025

Conversation

hayescr
Copy link
Contributor

@hayescr hayescr commented Dec 20, 2024

Resolves JP-3794

Closes #8938

This PR expands the use_source_posn option in extract_1d to work for curved, non-resampled cal and calints spectra (specifically for NIRSpec BOTS/MOS/FS and MIRI LRS fixed slit). It does so by updating the use_source_posn option to derives a source trace from WCS and expected source positions and performs a box extraction around the trace if an extraction width has been supplied. This PR also turns on the use_source_position by default for NIRSpec BOTS data, and adds a "position_offset" step parameter that allows users to supply cross-dispersion offsets to the pipeline extraction regions.

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

@hayescr
Copy link
Contributor Author

hayescr commented Jan 3, 2025

I've updated the original comment above and documentation to reflect that this is now an update to generalize the extract_1d use_source_posn to work with non-resampled data (thanks @melanieclarke for the modification!). I'll go ahead and pull this out of draft, let me know if there is anything else that should be changed/addressed.

@hayescr hayescr marked this pull request as ready for review January 3, 2025 21:43
@hayescr hayescr requested a review from a team as a code owner January 3, 2025 21:43
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.92%. Comparing base (e82bac9) to head (8acf6a5).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9022      +/-   ##
==========================================
+ Coverage   76.90%   76.92%   +0.02%     
==========================================
  Files         498      498              
  Lines       45769    45810      +41     
==========================================
+ Hits        35199    35240      +41     
  Misses      10570    10570              
Flag Coverage Δ *Carryforward flag
nightly 77.38% <ø> (-0.02%) ⬇️ Carriedforward from d24fc76

*This pull request uses carry forward flags. Click here to find out more.

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

@melanieclarke
Copy link
Collaborator

Thanks @hayescr! I'll start regression tests and tag someone else to review as well, since I contributed code.

@melanieclarke
Copy link
Collaborator

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.

There are some unrelated failures for imaging modes in the regression tests; otherwise, the only change is to the NIRSpec BOTS extracted spectrum in spec2, as expected. I think this is good to go.

@melanieclarke melanieclarke requested a review from tapastro January 6, 2025 15:20
@melanieclarke
Copy link
Collaborator

@tapastro - can you review as well, since I contributed some code for this one?

@melanieclarke
Copy link
Collaborator

Or maybe @jemorrison can review, since this touches on the PSF extraction work.

Copy link
Contributor

@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

Looks good to me, though I'm curious - why set use_source_posn to False for the regression test? It appears as though the new code will be used by default for BOTS data - do we expect a parameter reference file update that reflects this, setting it to False?

@melanieclarke
Copy link
Collaborator

Looks good to me, though I'm curious - why set use_source_posn to False for the regression test? It appears as though the new code will be used by default for BOTS data - do we expect a parameter reference file update that reflects this, setting it to False?

No, we want use_source_posn to be True for BOTS with these changes, from now on. I set it to False for that regression test because it was specifically meant to test custom extraction parameters with polynomial coefficients for source and background regions. The coefficients I originally defined for that test were not intended to be shifted by the source location -- they include the source position directly.

There is another BOTS regression test for spec2 that exercises the default extraction -- that one shows the intended changes.

@tapastro
Copy link
Contributor

tapastro commented Jan 7, 2025

No, we want use_source_posn to be True for BOTS with these changes, from now on. I set it to False for that regression test because it was specifically meant to test custom extraction parameters with polynomial coefficients for source and background regions. The coefficients I originally defined for that test were not intended to be shifted by the source location -- they include the source position directly.

There is another BOTS regression test for spec2 that exercises the default extraction -- that one shows the intended changes.

Ah, the downsides of reading the change snippets without context. 🙂 Thanks!

@jemorrison
Copy link
Collaborator

@melanieclarke I have a basic question. Where in the code does it know the data is not resampled and a curved traced should be extracted.

@hayescr
Copy link
Contributor Author

hayescr commented Jan 7, 2025

@jemorrison it doesn't explicitly check for resampled data to decide whether or not to extract a trace. In the case of resampled data the trace is straight and the trace array is filled with the "location" value that was previously used by the use_source_posn. So the behavior of use_source_posn is effectively the same, even though it may technically be using the "trace" for the extraction.

@jemorrison
Copy link
Collaborator

@hayescr Ok can you point me to were the shape of the trace is determined - mainly for the unresampled data.

return full_trace


def _miri_trace_from_wcs(shape, bounding_box, wcs_ref, source_ra, source_dec):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jemorrison here (_miri_trace_from_wcs) is the relevant function that calculates the trace for MIRI data. The equivalent function for NIRSpec is similar other than the differences in MIRI/NIRSpec WCS and can be found above (_nirspec_trace_from_wcs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here is where _miri_trace_from_wcs is called in location_from_wcs, in case it is useful to work backward and see what inputs are provided:

trace = _miri_trace_from_wcs(shape, bb, wcs, dithra, dithdec)

Copy link
Collaborator

@jemorrison jemorrison left a comment

Choose a reason for hiding this comment

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

This looks good to me. I really like how the 'trace' is extracted depending on the wcs information.

@melanieclarke melanieclarke merged commit 8b2d425 into spacetelescope:main Jan 9, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve extraction from unrectified NIRSpec data
4 participants