Skip to content

Conversation

@melanieclarke
Copy link
Contributor

@melanieclarke melanieclarke commented Dec 9, 2025

Resolves RTB-1027

Add a new step to the exposure level pipeline to address the WFI18 transient anomaly in the first read.

Note:

Tasks

  • request a review from someone specific, to avoid making the maintainers review every PR
  • add a build milestone, i.e. 24Q4_B15 (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#>.associations.rst
  • changes/<PR#>.scripts.rst
  • changes/<PR#>.mosaic_pipeline.rst
  • changes/<PR#>.skycell.rst

steps

  • changes/<PR#>.dq_init.rst
  • changes/<PR#>.saturation.rst
  • changes/<PR#>.refpix.rst
  • changes/<PR#>.linearity.rst
  • changes/<PR#>.dark_current.rst
  • changes/<PR#>.jump_detection.rst
  • changes/<PR#>.ramp_fitting.rst
  • changes/<PR#>.assign_wcs.rst
  • changes/<PR#>.flatfield.rst
  • changes/<PR#>.photom.rst
  • changes/<PR#>.flux.rst
  • changes/<PR#>.source_detection.rst
  • changes/<PR#>.tweakreg.rst
  • changes/<PR#>.skymatch.rst
  • changes/<PR#>.outlier_detection.rst
  • changes/<PR#>.resample.rst
  • changes/<PR#>.source_catalog.rst

@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

❌ Patch coverage is 96.90722% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.41%. Comparing base (3384c78) to head (520eb25).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
romancal/wfi18_transient/wfi18_transient_step.py 90.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2096      +/-   ##
==========================================
+ Coverage   79.22%   79.41%   +0.19%     
==========================================
  Files         140      143       +3     
  Lines        8696     8793      +97     
==========================================
+ Hits         6889     6983      +94     
- Misses       1807     1810       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@melanieclarke
Copy link
Contributor Author

melanieclarke commented Dec 9, 2025

Initial regtests here: https://github.com/spacetelescope/RegressionTests/actions/runs/20079904535

Tests in attempt 1 are failing for unrelated reasons. I'll wait until they clear up on main and run again.
Attempt 2 still shows some unrelated failures, but also shows errors in the new regression test because the truth files were made with the roman_datamodels on main, to include the new cal_step schema.

Running again with roman_datamodels on main:
https://github.com/spacetelescope/RegressionTests/actions/runs/20147695971

This one shows validation errors for the input to test_elp_tvac and test_wfi_dq_init, due to spacetelescope/roman_datamodels#613. The other failures are either unrelated (new CRDS context) or expected, from the new cal_step keyword added to output products.

@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file automation labels Dec 10, 2025
@melanieclarke melanieclarke force-pushed the wfi18_transient branch 2 times, most recently from 6c5d4e3 to 5591885 Compare December 11, 2025 17:16
@melanieclarke melanieclarke changed the title WIP RTB-1027: Add wfi18_transient step RTB-1027: Add wfi18_transient step Dec 12, 2025
@melanieclarke melanieclarke marked this pull request as ready for review December 12, 2025 20:34
@melanieclarke melanieclarke requested review from a team as code owners December 12, 2025 20:34
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This is great, thanks Melanie. I think this can go in. Let's see if Tim or Tyler have any suggestions for an appropriate flag bit and then merge later this week.

@melanieclarke
Copy link
Contributor Author

melanieclarke commented Dec 15, 2025

For posterity, here's what the correction looks like with the TVAC data used in the regression test. This is reduced with jump detection skipped because that also partially masks the transient signal:
strun roman_elp TVAC2_NOMOPS_TTNOISE_20240418084515_WFI18_uncal.asdf --steps.rampfit.use_ramp_jump_detection=False --steps.photom.skip=true --steps.tweakreg.skip=true --steps.source_catalog.skip=true

Left is the uncorrected data (wfi18_transient.skip=True), middle is just masking the first 1000 rows in the first read (wfi18_transient.mask_rows=True), right is the full double-exponential fit to the residuals and subtracted from the data.

wfi18_transient_correction

@melanieclarke
Copy link
Contributor Author

melanieclarke commented Dec 18, 2025

Regtests with spacetelescope/roman_datamodels#615:
https://github.com/spacetelescope/RegressionTests/actions/runs/20352731241

All diffs are as expected: the new cal_step keyword gets value N/A for WFI01 data, SKIPPED for all saturated data.

@melanieclarke
Copy link
Contributor Author

melanieclarke commented Jan 8, 2026

Rerun regtests following roman_datamodels update:
https://github.com/spacetelescope/RegressionTests/actions/runs/20855530239

There are a couple unrelated changes due to a new CRDS context, but otherwise changes look as expected: the new cal_step keyword is updated with N/A for WFI01 data, SKIPPED for all-saturated data.

@melanieclarke melanieclarke requested a review from schlafly January 8, 2026 21:15
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Thanks Melanie, looks great, merging!

@schlafly schlafly merged commit 1127a35 into spacetelescope:main Jan 9, 2026
24 checks passed
@melanieclarke melanieclarke deleted the wfi18_transient branch January 9, 2026 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation pipeline regression_testing stpipe testing Wide Field Instrument (WFI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants