-
Notifications
You must be signed in to change notification settings - Fork 167
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-1136: Compute scaling for WFSS background subtraction using error-weighted mean #8990
Merged
Merged
Changes from 25 commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
e302565
halfway through attempted fix
emolter be99137
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 41a7ab9
Fix sigma-clipping behavior and apply mask properly
emolter a858eb2
relax tolerance of test
emolter 0f74bce
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter b8d3d0f
apply sigma clipping on sci/bkg ratio
emolter f85926e
doc changes
emolter e655a0d
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter a0661e0
Split wfss into its own files, implemented iterative bkg sub for wfss
emolter 3b2f9f4
pass dispersion axis into scaling factor computer, add more tests
emolter 79c0acf
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 8df0f61
fixed dispersion axis calculation and zero-valued variance problem
emolter 39d2165
fixed dispersion axis computation again
emolter a61fa2e
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 76541c8
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 216df40
fractional rms improvement stopping criterion
emolter 964ec59
update docs for iterative background sub
emolter 6e2fda3
added warning filters
emolter 622183b
caught some typos during self review
emolter 3ee9cdd
fix ruff style issues
emolter a324873
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 2092a3b
fixed small style things in response to review comments
emolter 0d43f94
Made top-level unit tests, removed unnecessary looping thru all filters
emolter 380d089
made new parameter names more descriptive
emolter 0d1d172
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 54d93fe
Merge branch 'main' of https://github.com/spacetelescope/jwst into JP…
emolter 841c516
typo fix
emolter 8baec7e
update spec to make delta_rms_thresh have correct default
emolter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Compute scaling for WFSS background subtraction using error-weighted mean |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default described here does not match the default value in the step spec - from the discussion on the ticket, it appears as though this wasn't concretely specified. I think it would be reasonable to set to this to 1, but if you think 0 is more intuitive, I would not object. We just need the two locations to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an RMS stopping criterion, so only stopping when delta RMS is zero seems more intuitive to me. 1 could be a valid value, and it seems that INS wants to run up to the maxiter criterion by default. But I will check where the default documented here does not equal the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I also noticed this discrepancy, where in the code the default is set to 1. For the NIRISS tests I've performed, a default of 1 or 0 makes no difference as in both cases the code reaches the default maxiter of 5. But to be safe it could be 0 rather than 1.