Add Roman jump detection to ramp fitting #215
Merged
Conversation
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 comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAttention:
📢 Thoughts on this report? Let us know!. |
WilliamJamieson
commented
Sep 27, 2023
WilliamJamieson
force-pushed
the
feature/jump_detection
branch
from
8a0738e to
a8a6ab6
Compare
schlafly
reviewed
Oct 5, 2023
There was a problem hiding this comment.
This looks good to me. I made a bunch of minor comments. Some summary:
- Let's get rid of all of the _ma_table routines. We only ever need read_pattern; the ma_table pattern was something I made up for the simulator before read_pattern existed, and it doesn't need to continue existing.
- It will be good to see that this matches Sanjib's results, but the current tests look good and I'm optimistic.
- The final test in test_find_jumps would be more convincing if it were phrased in terms of the chi^2/dof. That's slightly annoying in that case since the degrees of freedom are 8 resultants - ~2 flagged - (2, 3, or 4 depending on where the jump landed). But I think it's not that bad and would make for a strong demo that both the ramps and their variances were good.
WilliamJamieson
force-pushed
the
feature/jump_detection
branch
from
09e01d9 to
ca58304
Compare
schlafly
approved these changes
Oct 6, 2023
There was a problem hiding this comment.
This looks great. Two questions:
- Can we modify dq in place? I have been trying to avoid making any new things that are nresultants x npixel in size elsewhere in the pipeline.
- I am missing some bit in one of the tests where I expect some false positives. Are there really none or am I confused?
tests/test_jump_cas22.py
Outdated
|
|
||
|
|
||
| RNG = np.random.default_rng(619) | ||
| ROMAN_READ_TIME = 3.04 |
There was a problem hiding this comment.
Really, I just feel like we'd like this to be mission independent, so I think I'm just asking to delete ROMAN_. But obviously this is minor and I don't feel strongly.
WilliamJamieson
force-pushed
the
feature/jump_detection
branch
from
6adb227 to
32f1d78
Compare
kmacdonald-stsci
approved these changes
Oct 10, 2023
WilliamJamieson
force-pushed
the
feature/jump_detection
branch
from
de09119 to
0178b75
Compare
6 tasks
WilliamJamieson
force-pushed
the
feature/jump_detection
branch
from
1f08368 to
40303d2
Compare
stscieisenhamer
approved these changes
Oct 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds the Roman jump detection to the roman ramp fitting algorithm.
Checklist
CHANGES.rst(either inBug FixesorChanges to API)