Skip to content

ForwardDispersion Model for MIRI and make MIRI and NIRISS use the same method#600

Merged
emolter merged 10 commits into
spacetelescope:mainfrom
jemorrison:miri_wfss_forward_model
Oct 15, 2025
Merged

ForwardDispersion Model for MIRI and make MIRI and NIRISS use the same method#600
emolter merged 10 commits into
spacetelescope:mainfrom
jemorrison:miri_wfss_forward_model

Conversation

@jemorrison
Copy link
Copy Markdown
Contributor

@jemorrison jemorrison commented Oct 7, 2025

Helps to resolve Resolves JP-3942

Closes #

This PR addresses updates the Forward model used for MIRWFSS. The Forward model for both NIRISS and MIRI are the same. The private module's name was changed to WFSSForwardGrismDistortion

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)
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

@jemorrison jemorrison requested a review from a team as a code owner October 7, 2025 20:20
@jemorrison jemorrison force-pushed the miri_wfss_forward_model branch from fde71fd to 387d1d2 Compare October 7, 2025 20:21
@jemorrison jemorrison marked this pull request as draft October 7, 2025 20:21
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.10%. Comparing base (2f233c2) to head (713e48f).
⚠️ Report is 104 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
- Coverage   90.13%   90.10%   -0.03%     
==========================================
  Files         111      111              
  Lines        5321     5306      -15     
==========================================
- Hits         4796     4781      -15     
  Misses        525      525              

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

@emolter
Copy link
Copy Markdown
Contributor

emolter commented Oct 8, 2025

I am fine with the name WFSSForwardDispersion.

I think the fact that MIRI and NIRISS but not NIRCam will share code means a bit of messiness is unavoidable in the naming. In my opinion it's not worth the trouble to change the names of anything non-MIRI-related, since those are already in active use.

@emolter
Copy link
Copy Markdown
Contributor

emolter commented Oct 8, 2025

Also since looking at the PR the only things that need to be changed are "private", these name changes should be non-controversial

@jemorrison jemorrison force-pushed the miri_wfss_forward_model branch from 4bce0f7 to 8e2deba Compare October 10, 2025 00:23
@jemorrison jemorrison marked this pull request as ready for review October 10, 2025 00:23
@emolter
Copy link
Copy Markdown
Contributor

emolter commented Oct 10, 2025

@jemorrison it looks like the docs build is failing because you need to add any new classes to the ignores list here:

for transform_base in [
(and please do remove any old ones)

Please do also add a changelog entry saying something about the forward dispersion model bugfix

@jemorrison
Copy link
Copy Markdown
Contributor Author

@emolter When you say do not remove old ones from conf.py - do you also mean do not remove _NIRISSForwardGrismDispersion ?
I changed that no to WFSSForwardGrismDispersion so the old one is no longer in stdatamodels - so it seems
I should remove that one. But I just wanted to double check.

@emolter
Copy link
Copy Markdown
Contributor

emolter commented Oct 13, 2025

No I didn't say don't remove old ones, please do remove any names that are now unused

@jemorrison
Copy link
Copy Markdown
Contributor Author

Sorry I read you comment before my coffee this morning. Got it now

Copy link
Copy Markdown
Contributor

@emolter emolter left a comment

Choose a reason for hiding this comment

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

Other than one suggestion about the changelog this looks good to me.

Comment thread changes/600.bugfix.rst Outdated
@emolter emolter self-requested a review October 14, 2025 19:53
@jemorrison
Copy link
Copy Markdown
Contributor Author

@emolter emolter merged commit c009663 into spacetelescope:main Oct 15, 2025
19 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.

2 participants