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-3669: 'OLS' Conditional Read Noise Recalculation for CHARGELOSS #8697

Merged
merged 8 commits into from
Sep 12, 2024

Conversation

kmacdonald-stsci
Copy link
Contributor

@kmacdonald-stsci kmacdonald-stsci commented Aug 8, 2024

Resolves JP-3669

This PR updates the code flow for read noise calculation. For ramps that have CHARGELOSS flagging, when calling the "OLS_C" algorithm, this recalculation is done in the C-extension in the ramp fitting code in STCAL, so the step code calculations defined in the JWST step code no longer needs to be run and will result in errors if it is run.

The step code that handles the recalculation of the read noise for CHARGELOSS flags is now only done when the "OLS" algorithm is used.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

  • 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
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

@kmacdonald-stsci
Copy link
Contributor Author

The regression testing using "-k rate" filter in the new github actions regression testing is here:

https://github.com/spacetelescope/RegressionTests/actions/runs/10309026450

This has expected failures and differences. A full regression test is currently being run on Jenkins:

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1639/

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 4.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 60.79%. Comparing base (2ab2da9) to head (f193e2a).

Files with missing lines Patch % Lines
jwst/ramp_fitting/ramp_fit_step.py 4.00% 24 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8697      +/-   ##
==========================================
- Coverage   60.79%   60.79%   -0.01%     
==========================================
  Files         373      373              
  Lines       38696    38697       +1     
==========================================
- Hits        23527    23525       -2     
- Misses      15169    15172       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

This looks fine to me.

The regression tests show a couple pixels different for NIRISS readnoise variances. Was this running with OLS_C and pointing to your stcal branch? Have you tracked down the differences?

CHANGES.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

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

LGTM, assuming the chargeloss calculation is indeed handled in the C code - I didn't check myself.

I agree with Melanie RE the changelog entry.

CHANGES.rst Outdated Show resolved Hide resolved
@kmacdonald-stsci
Copy link
Contributor Author

This looks fine to me.

The regression tests show a couple pixels different for NIRISS readnoise variances. Was this running with OLS_C and pointing to your stcal branch? Have you tracked down the differences?

Yes, the regression test using OLS_C. The differences are known and expected, due to the known segmentation bug in the OLS code.

CHANGES.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

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

Linked to spacetelescope/stcal#275

Looks good to me! I've successfully tested that:

  1. Runtime improves as expected in test case jw02079004003_03101_00001_nis. In the old version it takes about 2.5 minutes to run the relevant piece of the RN recalculation, while with this PR it takes about 2.5 seconds.

  2. RN variances stay as expected in test case jw01510001001_02108_00001_nis using both the OLS and OLS_C algorithms.

  3. Checked that the SCI and variance arrays did not change for other instruments that don't use charge migration.

CHANGES.rst Outdated Show resolved Hide resolved
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.

I think this should be good to go, given David's tests and the clean regression tests. I'll approve now, but it does need a conflict fixed in the change log and the stcal change needs to be merged first. When that's done, go ahead and merge this one in too.

merge with main had unintended addition
final merge correction
@tapastro tapastro added this to the Build 11.1 milestone Sep 10, 2024
@tapastro
Copy link
Contributor

Running regression tests before merging: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1701/

@tapastro tapastro merged commit 291b375 into spacetelescope:master Sep 12, 2024
23 of 24 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.

5 participants