Skip to content

JP-4313: Use explicit read_times in JWST model in ramp fit#546

Open
melanieclarke wants to merge 1 commit into
spacetelescope:mainfrom
melanieclarke:jp-4313
Open

JP-4313: Use explicit read_times in JWST model in ramp fit#546
melanieclarke wants to merge 1 commit into
spacetelescope:mainfrom
melanieclarke:jp-4313

Conversation

@melanieclarke

@melanieclarke melanieclarke commented May 20, 2026

Copy link
Copy Markdown
Contributor

Toward JP-4313

For coordination with spacetelescope/jwst#10534:
Use explicit read times for JWST data if available. This is intended to support new multistripe modes with repeated in-frame reads of the same detector area.

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see changelog readme for instructions)
    • if your change breaks existing functionality, also add a changes/<PR#>.breaking.rst news fragment
  • run regression tests with this branch installed ("git+https://github.com/<fork>/stcal@<branch>")

@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.79%. Comparing base (a04ec58) to head (b2da8cd).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/stcal/ramp_fitting/ramp_fit.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
- Coverage   91.80%   91.79%   -0.01%     
==========================================
  Files          63       63              
  Lines        8855     8859       +4     
==========================================
+ Hits         8129     8132       +3     
- Misses        726      727       +1     

☔ 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 melanieclarke changed the title WIP JP-4313: Use explicit read_times in JWST model in ramp fit JP-4313: Use explicit read_times in JWST model in ramp fit May 28, 2026
@melanieclarke

melanieclarke commented May 28, 2026

Copy link
Copy Markdown
Contributor Author

@melanieclarke melanieclarke marked this pull request as ready for review June 2, 2026 16:30
@melanieclarke melanieclarke requested a review from a team as a code owner June 2, 2026 16:30
@schlafly

schlafly commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

I probably need to review better what romancal does, but I'm a little confused. The old version took read_pattern from the input model and converted it to a list of lists and put it at read_pattern on ramp_data. The new version no longer looks for read pattern at all and instead looks for read_times, to my eyes basically redefining read_pattern -> read_times for models but not for the ramp data. Why can't Webb use the old name? I would have naively expected romancal to break since it uses read_pattern as the name
https://github.com/spacetelescope/romancal/blob/main/romancal/ramp_fitting/ramp_fit_step.py#L259
and haven't yet figured out why it still works with the new name.

@melanieclarke

melanieclarke commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

I probably need to review better what romancal does, but I'm a little confused. The old version took read_pattern from the input model and converted it to a list of lists and put it at read_pattern on ramp_data. The new version no longer looks for read pattern at all and instead looks for read_times, to my eyes basically redefining read_pattern -> read_times for models but not for the ramp data. Why can't Webb use the old name? I would have naively expected romancal to break since it uses read_pattern as the name https://github.com/spacetelescope/romancal/blob/main/romancal/ramp_fitting/ramp_fit_step.py#L259 and haven't yet figured out why it still works with the new name.

Romancal doesn't use create_ramp_fit_class, it creates the ramp fit class directly. The ramp data class takes a list of lists of read times as the "read_pattern", so even if romancal did use create_ramp_fit_class, it wouldn't be correct to directly read meta.exposure.read_pattern into the ramp class read_pattern attribute.

I'd like for Webb to use meta.exposure.read_times instead of meta.exposure.read_pattern in the datamodel because we need to directly store a list of lists of read times, not a frame averaging pattern.

At some point, I think it would make sense to move create_ramp_fit_class from stcal to jwst to make it clearer that we each need to set up our own ramp fit classes, but I didn't want to do it here.

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.

2 participants