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-3087: Fix zeroing of NaNs in straylight #7455

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

drlaw1558
Copy link
Collaborator

@drlaw1558 drlaw1558 commented Feb 6, 2023

This PR fixes a new issue where the MRS straylight routine converts NaNs to zeros. Previously this was a safety catch in case any NaNs were present that were not meant to be there, but now that the rate file is meant to contain NaNs this behaviour is undesirable. This PR modifies straylight to only do the NaN-to-zero conversion in a local copy of the input used for the straylight computation itself.

Resolves JP-3087

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

@drlaw1558 drlaw1558 requested a review from jemorrison February 6, 2023 20:30
@hbushouse hbushouse added this to the Build 9.2 milestone Feb 8, 2023
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.

Code looks fine. Would it be way too difficult to update the underlying model code to be able to cope with NaNs, as opposed to replacing them before sending down?

jwst/straylight/straylight.py Outdated Show resolved Hide resolved
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.

LGTM

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Base: 77.90% // Head: 77.90% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (d6f00c0) compared to base (beb200d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7455   +/-   ##
=======================================
  Coverage   77.90%   77.90%           
=======================================
  Files         455      455           
  Lines       36975    36977    +2     
=======================================
+ Hits        28806    28808    +2     
  Misses       8169     8169           
Flag Coverage Δ
nightly 77.89% <100.00%> (+<0.01%) ⬆️
unit 50.43% <0.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
jwst/straylight/straylight.py 84.76% <100.00%> (+0.29%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@jemorrison jemorrison left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@hbushouse
Copy link
Collaborator

@hbushouse
Copy link
Collaborator

regtest run showed 1 unrelated failure, due to recent changes in stcal/ramp_fitting. So it's good.

@hbushouse hbushouse merged commit 171b3b7 into spacetelescope:master Feb 13, 2023
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.

3 participants