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

Refpix bug fix #7731

Merged
merged 6 commits into from
Jul 17, 2023
Merged

Refpix bug fix #7731

merged 6 commits into from
Jul 17, 2023

Conversation

penaguerrero
Copy link
Contributor

@penaguerrero penaguerrero commented Jul 15, 2023

This PR resolves a potential crash due to an empty list. Also added a factor to mitigate the overcorrection due to suspicious intermittently bad reference pixels.

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

@penaguerrero penaguerrero requested a review from a team as a code owner July 15, 2023 15:17
@penaguerrero penaguerrero changed the title Refpix bug fix WIP: Refpix bug fix Jul 15, 2023
@codecov
Copy link

codecov bot commented Jul 15, 2023

Codecov Report

Patch coverage: 11.11% and project coverage change: -0.20 ⚠️

Comparison is base (1e4f6c5) 76.60% compared to head (851559f) 76.41%.

❗ Current head 851559f differs from pull request most recent head c09dc90. Consider uploading reports for the commit c09dc90 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7731      +/-   ##
==========================================
- Coverage   76.60%   76.41%   -0.20%     
==========================================
  Files         456      456              
  Lines       36947    36950       +3     
==========================================
- Hits        28303    28235      -68     
- Misses       8644     8715      +71     
Flag Coverage Δ *Carryforward flag
nightly 77.43% <ø> (-0.01%) ⬇️ Carriedforward from 8e20edb

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
jwst/refpix/refpix_step.py 73.17% <0.00%> (-2.44%) ⬇️
jwst/refpix/irs2_subtract_reference.py 67.40% <12.50%> (-19.39%) ⬇️

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

@penaguerrero penaguerrero changed the title WIP: Refpix bug fix Refpix bug fix Jul 15, 2023
@hbushouse hbushouse added bug IRS2 readout NIRSpec IRS2 readout mode NIRSPEC labels Jul 17, 2023
@hbushouse hbushouse added this to the Build 9.3 milestone Jul 17, 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 updates look OK, but getting CI failures, because the "test_rm_intermittent_badpix.py" unit test module needs to be updated to include the new argument in the function call. Also need an update to docs for the new step argument.

@hbushouse
Copy link
Collaborator

Mike reported 3 datasets that caused an error for this condition. I've copied the uncal files to /grp/jwst/ssb/chartreuse/dms_bugs/jp-3157/ for testing purposes (to see if this PR avoids the error).

@github-actions github-actions bot removed the NIRSPEC label Jul 17, 2023
@penaguerrero
Copy link
Contributor Author

Added documentation, fixed test, and tested with data from central store. All good from my end.

@hbushouse
Copy link
Collaborator

@hbushouse
Copy link
Collaborator

Regression test results show minor differences in pixel values starting with and downstream from the refpix step, which I'm assuming are expected differences due to this change. No errors of any kind.

@hbushouse hbushouse merged commit ca4ab10 into spacetelescope:master Jul 17, 2023
mairanteodoro pushed a commit to mairanteodoro/jwst that referenced this pull request Jul 19, 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.

2 participants