Skip to content

Conversation

@emolter
Copy link
Collaborator

@emolter emolter commented May 2, 2025

Resolves JP-3941

This PR refactors the white light step to return sensible results for NIRISS data.

On main, and e.g. in the current truth files of our regtests (see e.g. truth/test_niriss_soss_stages/jw01091-o002_t001_niriss_clear-gr700xd-substrip256_whtlt.ecsv, the _whtlt.ecsv files have two major issues:

  • The times column contains repeats of the 0th value in a file, and only changes for different input files in the association
  • The columns are not split by spectral order, meaning that the outputs from multiple orders are interleaved

This PR fixes both of those issues - compare the file mentioned above with the one attached to the JIRA ticket, which came from the same input association.

The control logic of the step is also simplified.

Outputs are unaffected for NIRCam tsgrism and MIRI LRS slitless TSO data.

Tasks

  • If you have a specific reviewer in mind, tag them.
  • add a build milestone, i.e. Build 12.0 (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 changelog readme for instructions)
    • 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

@emolter
Copy link
Collaborator Author

emolter commented May 2, 2025

regtest round started here: https://github.com/spacetelescope/RegressionTests/actions/runs/14802258787

Only one failure, as expected, in jwst.regtest.test_niriss_soss::test_niriss_soss_stage3_whtlt

@codecov
Copy link

codecov bot commented May 2, 2025

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.93%. Comparing base (677dce1) to head (85d1552).
⚠️ Report is 676 commits behind head on main.

Files with missing lines Patch % Lines
jwst/white_light/white_light.py 97.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9431      +/-   ##
==========================================
+ Coverage   77.92%   77.93%   +0.01%     
==========================================
  Files         362      362              
  Lines       36283    36252      -31     
==========================================
- Hits        28272    28253      -19     
+ Misses       8011     7999      -12     

☔ 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 emolter changed the title JP-3941: Refactor white_light to get correct time stamps and spectral orders JP-3941: Refactor white_light to get correct time stamps and spectral orders for NIRISS May 2, 2025
@emolter emolter marked this pull request as ready for review May 2, 2025 22:03
@emolter emolter requested review from a team as code owners May 2, 2025 22:03
@emolter emolter assigned AarynnCarter and unassigned AarynnCarter May 2, 2025
@emolter emolter requested a review from AarynnCarter May 2, 2025 22:05
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.

The output format looks good to me but we'll wait for INS review.

This will need integration with #9430 - this PR should go in first, then I will update for the new TSO format.

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.

A couple minor comments from working with these changes to update them for #9430

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.

LGTM, but we should make sure INS is happy with the format before merging.

@emolter
Copy link
Collaborator Author

emolter commented May 7, 2025

Sounds good. It seems like they are generally unhappy with the step output. My plan is to just write a comment on the JIRA ticket asking if this is at least an improvement, and to start a separate discussion about the actual table contents... does that sound good?

@melanieclarke
Copy link
Collaborator

Sounds good. It seems like they are generally unhappy with the step output. My plan is to just write a comment on the JIRA ticket asking if this is at least an improvement, and to start a separate discussion about the actual table contents... does that sound good?

Perfect. Thanks!

@emolter emolter requested a review from drlaw1558 May 7, 2025 17:43
Copy link
Contributor

@drlaw1558 drlaw1558 left a comment

Choose a reason for hiding this comment

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

LG2M

@melanieclarke melanieclarke enabled auto-merge May 7, 2025 21:32
@melanieclarke melanieclarke merged commit 0d4bb7f into spacetelescope:main May 7, 2025
19 checks passed
@emolter emolter deleted the JP-3941 branch May 8, 2025 13:10
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.

4 participants