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.

Sign up for GitHub

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

Jump to bottom

JP-2693: Flag groups do_not_use after detected ramp jumps #6943

Merged
merged 6 commits into from
Aug 17, 2022
Merged

JP-2693: Flag groups do_not_use after detected ramp jumps #6943

merged 6 commits into from
Aug 17, 2022

Conversation

karllark
Copy link
Collaborator

@karllark karllark commented Jul 26, 2022 •
edited by jdavies-st
Loading

Resolves JP-2693

This PR implements the enhancement to the jump step where groups after detected jumps are flagged do_not_use. This "corrects" transients that have been seen to make the slope after a jump different than before. This PR allows for flagging to be different for two different jump thresholds.

The main changes are in spacetelescope/stcal#110 and this PR provides the needed interface for jwst.

Checklist

  • 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)

@karllark karllark marked this pull request as draft July 26, 2022 20:20
@github-actions github-actions bot added the jump label Jul 26, 2022
@dmggh
Copy link
Contributor

dmggh commented Jul 27, 2022

I just changed ticket number '2963' -> '2693' in description.

@karllark karllark force-pushed the jump_transient_flag branch from 4476592 to 7d71a24 Compare August 10, 2022 15:12
@karllark
Copy link
Collaborator Author

If understand things correctly, the tests are failing as the version of stcal used for the tests does not include the changes needed for this PR. After the stcal PR has been merged, then maybe the tests will pass.

The tests pass on my computer when the appropriate branch on stscal (jump_transient_flag) is used.

@karllark karllark force-pushed the jump_transient_flag branch from 8460ce2 to 5e2b61e Compare August 10, 2022 17:15
@karllark karllark marked this pull request as ready for review August 10, 2022 17:15
@karllark
Copy link
Collaborator Author

Ready for review. Likely, the corresponding PR in stcal should be reviewed at the same time.

@hbushouse hbushouse added this to the Build 9.0 milestone Aug 12, 2022
cshanahan1
cshanahan1 reviewed Aug 12, 2022
View reviewed changes
docs/jwst/jump/arguments.rst Show resolved Hide resolved
docs/jwst/jump/description.rst Show resolved Hide resolved
jwst/jump/jump.py Show resolved Hide resolved
jwst/jump/jump.py Show resolved Hide resolved
hbushouse
hbushouse reviewed Aug 12, 2022
View reviewed changes
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump.py Outdated Show resolved Hide resolved
jwst/jump/jump_step.py Show resolved Hide resolved
jwst/jump/jump_step.py Show resolved Hide resolved
hbushouse
hbushouse approved these changes Aug 17, 2022
View reviewed changes
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. All review comments have been addressed.

@hbushouse
Copy link
Collaborator

@cshanahan1 are you good with this now that all review comments have been addressed?

@hbushouse
Copy link
Collaborator

@karllark I think this PR branch may need to be rebased from master. Looks like conflicts are preventing the CI tests from running.

@karllark karllark force-pushed the jump_transient_flag branch from 0fb895c to 625d634 Compare August 17, 2022 14:02
@karllark
Copy link
Collaborator Author

@karllark I think this PR branch may need to be rebased from master. Looks like conflicts are preventing the CI tests from running.

Done.

@hbushouse
Copy link
Collaborator

I think the remaining CI failures, which are confined to ramp_fitting unit tests, are due to other PR's included in the stcal 1.1.0 release, and maybe fixed by #6949 once it gets merged. Retesting that PR right now.

@codecov
Copy link

codecov bot commented Aug 17, 2022 •
edited
Loading

Codecov Report

Merging #6943 (abf8ce9) into master (e8318e6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6943      +/-   ##
==========================================
+ Coverage   79.24%   79.26%   +0.01%     
==========================================
  Files         414      414              
  Lines       37478    37508      +30     
==========================================
+ Hits        29701    29731      +30     
  Misses       7777     7777
Flag Coverage Δ *Carryforward flag
nightly 79.23% <100.00%> (ø) Carriedforward from e8318e6
unit 53.09% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
jwst/jump/jump.py 100.00% <100.00%> (ø)
jwst/jump/jump_step.py 100.00% <100.00%> (ø)

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

@hbushouse
Copy link
Collaborator

OK, #6949 has now been merged to update the truth values in the unit tests that were causing CI failures. So one more rebase to kickoff the tests again.

@karllark karllark force-pushed the jump_transient_flag branch from 625d634 to abf8ce9 Compare August 17, 2022 19:07
@hbushouse hbushouse merged commit 2ae48e3 into spacetelescope:master Aug 17, 2022
@hbushouse hbushouse added the 8.1 patch PR candidate for an 8.1 patch release label Aug 31, 2022
zacharyburnett pushed a commit to zacharyburnett/jwst that referenced this pull request Aug 31, 2022
@karllark @zacharyburnett
JP-2693: Flag groups do_not_use after detected ramp jumps (spaceteles…
bfba7c6 
…cope#6943)

* passing after_jump parameters through to stcal

* adding after_jump test

* adding to change log

* adding after ramp flagging docs

* fixing docs

* Responding to PR comments

(cherry picked from commit 2ae48e3)
@hbushouse hbushouse modified the milestones: Build 9.0, Build 8.1.2 Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cshanahan1 cshanahan1 cshanahan1 approved these changes

@hbushouse hbushouse hbushouse approved these changes

Assignees
No one assigned
Labels
8.1 patch PR candidate for an 8.1 patch release documentation jump testing
Projects
None yet
Milestone
Build 8.1.2
Development

Successfully merging this pull request may close these issues.
4 participants
@karllark @dmggh @hbushouse @cshanahan1