-
Notifications
You must be signed in to change notification settings - Fork 1
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
DM-39243: Add an initial cell-based coadd task #33
Conversation
e7fe45b
to
b64c165
Compare
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.
I've left a lot of line comments, and I think you already know there are some docstrings missing. But don't let that distract that this is almost all of what we want, and that makes it a very big step on a very important thing.
for cellInfo in skyInfo.patchInfo: | ||
stacker = AccumulatorMeanStack( | ||
shape=(cellInfo.outer_bbox.getDimensions().x, cellInfo.outer_bbox.getDimensions().y), | ||
bit_mask_value=statsCtrl.getAndMask(), |
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.
If this is all we're using from StatisticsControl
, I'd be inclined to drop it and reimplement this method in Python.
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.
We still pass the StatisticsControl
object to _compute_weight
above that uses a clipped mean of the variance plane to compute the weight.
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.
I've replaced this specific use of statsCtrl
with the line that set its AndMask
in the first place. However, statsCtrl
in the code is to stay for now, until we can robustly estimate weight in a later ticket.
) | ||
badMaskPlanes = ListField[str]( | ||
doc="Mask planes that, if set, the corresponding pixel would not be included in the coadd.", | ||
default=("BAD", "EDGE", "NO_DATA", "SAT"), |
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.
What does this actually mean for cell-based coadds, where we need to use a consistent set of epochs for all pixels in the coadd? I thought we'd have to interpolate mask planes like these on the warps (here, after reading them, would be fine), and then just add the interpolated values as if they were real (after thresholding on the number of such pixels, etc.).
The implementation seems to happen inside AccumulatorMeanStack
(and I'm not familiar with its innards), so is this something the DESC coadds are also doing?
If we do end up dropping this option - or using it to interpolate warps after reading them in - I'm not sure we'd be getting much out of AccumulatorMeanStack
, especially after we start populating SingleCellCoadd.inputs
, since that will given us another accumulated-as-we-go data structure with weight information we can divide by later. (We certainly don't have to take this step now.)
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.
These are the mask planes that should count towards the calculation the fraction of masked pixels. I updated the doc to reflect this. This, and max_maskfrac
are currently just placeholders until we do the interpolation, which we don't right now.
The DESC coadds use AccumulatorMeanStack
as well, but the interpolation does not happen within it. The DESC code interpolates the bad pixels on the calexp
before warping and pass them onto the AccumulatorMeanStack
.
d84cf27
to
37ef432
Compare
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.
I think we're on the same page on how to resolve all issues raised here, so I'm happy for you to proceed and just ping me if something comes up.
37ef432
to
4010cf0
Compare
da4b5d7
to
9c29655
Compare
9c29655
to
7dfee69
Compare
This does a bunch of refactoring to the existing
AssembleCoadd
andCompareWarpAssembleCoadd
tasks to switch allow for old type of coadds and the new cell-based coadds. The task constructs the coadds using warps and does not use any DESC code for now. This places the infrastructure in place to make further developments.Edit: The old refactoring code described above has been punted to DM-41632. This is clean(er) stand-alone pipeline task without relying on the legacy CoaddBase code base.