Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-3451: Include zero-valued pixels in difference calculation #8038

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

tapastro
Copy link
Contributor

@tapastro tapastro commented Oct 30, 2023

Resolves JP-3451

Closes #8037

This PR addresses regression test errors for NIRISS SOSS after a recent delivery of updated reference files. This fix allows the difference calculation inside the bad dispersion direction method to see zero-valued pixels, which will allow it to overwrite them before throwing an error.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@tapastro tapastro added this to the Build 10.1 milestone Oct 30, 2023
@tapastro tapastro self-assigned this Oct 30, 2023
@tapastro tapastro requested a review from a team as a code owner October 30, 2023 21:12
@tapastro
Copy link
Contributor Author

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Files Coverage Δ
jwst/extract_1d/soss_extract/atoca_utils.py 69.52% <0.00%> (-0.13%) ⬇️

📢 Thoughts on this report? Let us know!.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

No idea if this is a correct solution or not, but it seems to work. Needs a change log entry.

@tapastro
Copy link
Contributor Author

tapastro commented Nov 2, 2023

Changelog update added; regression test shows failure with ~16% of pixels showing a difference. In reality it's 100% of science pixels, and it's expected because the updates here (with 1144) include an update to the photom ref file for SOSS that corrects a MJy -> Jy unit error, leading to ~1.e6 differences.

Copy link
Collaborator

@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

No idea if this is a correct solution or not, but it seems to work. Needs a change log entry.

@hbushouse
Copy link
Collaborator

@tapastro Is it OK to go ahead and merge this now, or are you waiting for updated reference files or anything like that?

@tapastro
Copy link
Contributor Author

tapastro commented Nov 9, 2023

I believe this should work for the old (current) ref files, and was tested above to work with the context that was skipped, such that the team can re-deliver the updates. I can run the regtests again on the current newest context, which does not contain the updates.

@tapastro
Copy link
Contributor Author

tapastro commented Nov 9, 2023

Re-run of regtests shows no failures: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1049/

@hbushouse hbushouse merged commit 9dfaa37 into spacetelescope:master Nov 10, 2023
21 of 23 checks passed
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.

Address SOSS regression test errors from new ref file delivery
2 participants