Skip to content

JP-3844: x1d.fits headers for the LRS should include PA_APER#601

Merged
melanieclarke merged 9 commits into
spacetelescope:mainfrom
penaguerrero:x1d_add_pointing
Nov 3, 2025
Merged

JP-3844: x1d.fits headers for the LRS should include PA_APER#601
melanieclarke merged 9 commits into
spacetelescope:mainfrom
penaguerrero:x1d_add_pointing

Conversation

@penaguerrero

@penaguerrero penaguerrero commented Oct 9, 2025

Copy link
Copy Markdown
Collaborator

Resolves JP-3844

This PR addresses adding PA_APER keyword to the EXTRACT1D extension headers

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 below for change types)
    • run jwst regression tests with this branch installed ("git+https://github.com/<fork>/stdatamodels@<branch>")
news fragment change types...
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@codecov

codecov Bot commented Oct 9, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.41%. Comparing base (c96cfd4) to head (7677bbb).
⚠️ Report is 88 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #601   +/-   ##
=======================================
  Coverage   89.41%   89.41%           
=======================================
  Files          99       99           
  Lines        4854     4854           
=======================================
  Hits         4340     4340           
  Misses        514      514           

☔ 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.

@penaguerrero penaguerrero marked this pull request as ready for review October 9, 2025 20:17
@penaguerrero penaguerrero requested a review from a team as a code owner October 9, 2025 20:17
@penaguerrero

Copy link
Copy Markdown
Collaborator Author

reg test: https://github.com/spacetelescope/RegressionTests/actions/runs/18387917675/job/52391172906

Failures are all expected since the x1d files now have one additional keyword.

@emolter

emolter commented Oct 10, 2025

Copy link
Copy Markdown
Contributor

Why do the name and the ticket both talk about PA_APER but the PR is adding PA_V3?

@penaguerrero

Copy link
Copy Markdown
Collaborator Author

thanks @emolter, PA_V3 and PA_APER are very similar. I made changes.

@braingram braingram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for working on this. The changelog doesn't match the schema change.

Please also update the PR description. It incorrectly mentions PA_V3.

Also, please re-run the regtests (as they were run when this PR added PA_V3).

Comment thread changes/601.feature.rst Outdated
Co-authored-by: Brett Graham <brettgraham@gmail.com>
@penaguerrero

Copy link
Copy Markdown
Collaborator Author

@emolter emolter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this looks fine, but I'd like to wait for an expert to review the JWST changes.

@braingram

Copy link
Copy Markdown
Collaborator

new reg test running: https://github.com/spacetelescope/RegressionTests/actions/workflows/jwst.yml

The link is to the general action page. Is there a link for an updated run? Thanks!

@braingram braingram dismissed their stale review October 16, 2025 20:36

Changes addressed

@melanieclarke melanieclarke changed the title JP-3844: x1d.fits headers for the LRS should include PA_APER (and possibly more) JP-3844: x1d.fits headers for the LRS should include PA_APER Oct 23, 2025
@melanieclarke

melanieclarke commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

This looks fine to me so far. Can you please run regression tests with jwst on main and link them here?

@melanieclarke

Copy link
Copy Markdown
Contributor

@emolter - do we need to add position_angle to WFSS handling, i.e. in wfss_specmeta.schema?

@emolter

emolter commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

I thought this piece of metadata was only helpful for long-slit spectra, what am I missing?

@melanieclarke

Copy link
Copy Markdown
Contributor

I thought this piece of metadata was only helpful for long-slit spectra, what am I missing?

Just checking to make sure we've covered all the bases, since WFSS has a different set of metadata than all the other modes. If it's not needed, then no action necessary.

@emolter

emolter commented Oct 23, 2025

Copy link
Copy Markdown
Contributor

I'm still not really a WFSS science data expert, but I can't imagine how it would be needed

@penaguerrero

Copy link
Copy Markdown
Collaborator Author

@penaguerrero

Copy link
Copy Markdown
Collaborator Author

The failed tests are expected due to the new keywords in the x1d files.

@melanieclarke

Copy link
Copy Markdown
Contributor

@penaguerrero - can you please run regression tests with jwst on main? If those are clear, we can get this change merged first, then finish up the jwst PR.

@penaguerrero

Copy link
Copy Markdown
Collaborator Author

reg test on jwst main with this branch for stdatamodels: https://github.com/spacetelescope/RegressionTests/actions/runs/19044861177

@melanieclarke melanieclarke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Regtests passing with jwst on main. I'll get this one merged when CI completes.

@melanieclarke melanieclarke merged commit 5ef6d79 into spacetelescope:main Nov 3, 2025
19 of 20 checks passed
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.

5 participants