-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow SnowblindStep to run on _rate and _rateints #1
Conversation
Thanks for the PR @gbrammer! Is the idea here to run it after Is running snowball flagging on the _rate file what is currently done in |
There are PEP8 style failures https://github.com/mpi-astronomy/snowblind/pull/1/checks#step:5:18 |
Yes, grizli just flags the big snowball blobs in the |
OK, ci tests pass locally now. |
The last fix was for the option to do a contraction with a negative The main reason I'm working on this is that rerunning
takes forever and a huge amount of memory. Doing the flagging directly on the MAST |
Is there anything to consider before merging this into a new pip release? It's been working very well for some updates on grizli and it would be helpful to include a release version of |
Hi @gbrammer, sorry for the radio silence. Euclid is keeping me pretty busy! Yeah, let's get this merged. I think it's almost there. One question is why have a custom |
Also, I'd like to try to convince you that you should be running But if you flag at the group level, you will only be flagging 1 or 2 groups out of 10 that are eventually used to compute the _rate. In the most typical case where NFRAMES>1, that's 2 groups flagged, so your final drizzled depth at those pixels is not 3/4 but 38/40 19/20 of the original depth. And for long exposures with lots of snowballs, this can have significant effect on final S/N for faint sources. Of course at the center of the snowball which is saturated (and the ~2 pixel ring around it where you get charge migration) you will still lose all the signal in the ramp, but it's still likely that there are some good groups beforehand and those will be useful. But the number of saturated pixels relative to the size of the expanded snowball mask is tiny. The animated GIF below is an example illustrating this with some NIRCam LONG data in one of the CEERS fields. 10 groups, 1 integration, 4 dithers. Final mosaic drizzled after |
I added this just to make the flagging more flexible. It defaults to the behavior you describe here, but when flagging on the I use the 1024 bit as my own |
I certainly accept your point, but the CPU/memory resources required for |
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.
Thanks for this Gabe. Glad this expands the functionality. I note only the one problem below of SATURATED not being propagated to the _rate files properly, which might make it less effective recognizing snowballs.
else: | ||
self._has_groups = False | ||
bool_jump = (jump.dq & JUMP_DET) == JUMP_DET | ||
bool_sat = (jump.dq & SATURATED) == SATURATED |
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.
In the case of _rate files currently, only pixels that are saturated in all groups get the SATURATED flag. This is not good in my view, but what it means in practice is that a snowball saturates some pixels mid-exposure and the _rate file has a slope computed for these pixels due to the first part of the ramp, and the DQ only shows JUMP_DET in the area around the core. This may make it more difficult to detect snowballs.
Btw, there's an open issue to have this changed. See spacetelescope/jwst#8124
If the snowball lands in the 1st group, then the core may be saturated (it might not be if NFRAMES is large) for all groups, and so one would get NaNs in the core in the _rate file. Not sure of the best way to handle this.
Add
SnowblindStep
handling forImageModel
andCubeModel
inputs, e.g.,rate
andrateints
products. The logic for deciding what to do is whether or not the datamodel has agroupdq
ordq
attribute.The update also includes a new parameter
to allow the user to set a custom integer flag on the dilated jumps.
Basic tests also included, based on the original tests for the
RampModel
inputs.