DM-52724: Allow for cell coadds with edges#52
Conversation
| """Second order moments of the PSF.""" | ||
|
|
||
| psf_shape_flag: bool | ||
| """Flag indicating whether the PSF shape is valid.""" |
There was a problem hiding this comment.
Is this just a shortcut for whether psf_shape has a negative determinant?
There was a problem hiding this comment.
No, this indicates a measurement failure. Perhaps I should change valid to success? We don't check for negative determinant at this point since the statistic is on the trace.
There was a problem hiding this comment.
Yeah, I think success is a little clearer.
| edge_width=1, | ||
| edge_mask_name="CELL_EDGE", | ||
| ): | ||
| """Set a mask bit indicating the inner cell edges. |
There was a problem hiding this comment.
Is this mask just for internal use during coaddition, or is it something the user sees? If the latter, what does it mean for users?
There was a problem hiding this comment.
This is for the user to know where the cell boundaries are when visualizing a stitched coadd. I could see the measurement framework using it eventually, but for now, these are for eyeballing only. I'm happy to not call this method in __init__ and have the user call it if we want to save a bit.
| assert len(aperture_correction_hdu) == 0 or len(aperture_correction_hdu) == len( | ||
| data | ||
| ), "Aperture correction map is not available for all cells." | ||
| if len(aperture_correction_hdu) == 0 or len(aperture_correction_hdu) == len(data): |
There was a problem hiding this comment.
What condition does the == len(data) clause represent?
There was a problem hiding this comment.
Good question. I don't remember why I put in that, and it doesn't make sense to me now after going through the code as is in this ticket, or as it was when I first put it in in #41 . Removing the second clause.
| __all__ = ("CoaddInputs", "SingleCellCoadd",) | ||
|
|
||
| from collections.abc import Iterable, Set | ||
| from collections import OrderedDict |
There was a problem hiding this comment.
Regular Python dict has been ordered since Python 3.7; there's no need to use OrderedDict anymore.
arunkannawadi
force-pushed
the
tickets/DM-52724
branch
3 times, most recently
from
3c04363 to
0e9f5c6
Compare
|
Jim, requesting review of the most recent commit that adds the polygon vertices to a separate table. |
| polygon_vertices_array = [] | ||
| for poly in multiple_cell_coadd.common.visit_polygons.values(): | ||
| vertices = poly.getVertices() + poly.getVertices() | ||
| vertices = vertices[:6] |
There was a problem hiding this comment.
A variable length array would have been more natural but since we had discussed that is suboptimal in terms for implementation details, I'm using a fixed length array here. This means for polygons with fewer than 6 vertices, there would be repetition. I cannot think of a scenario where the overlapping polygon will have more than 6 vertices.
There was a problem hiding this comment.
I don't recall discussion variable-length vs. fixed-length arrays. Variable-length does mean the data gets shunted down to the end of the HDU, which might be a minor inefficiency but probably not a large one.
Do you know if the duplicate vertices automatically get dropped on read? If not, having a separate field for the true size might be better, even if we keep the maximum size fixed.
There was a problem hiding this comment.
They don't get dropped automatically but it didn't look like giving the same point twice seemed to have an effect in practice (its __repr__ is different for sure). I was thinking of using set before constructing the Polygon here but keeping track of number of indices is a good idea.
Trying to put a VLA in one of the intermediate HDUs broke the ability to read the following ones. Now that makes sense.
| hdu_list.append(aperture_correction_hdu) | ||
|
|
||
| hdu_list.writeto(filename, overwrite=overwrite) | ||
| hdu_list.writeto(filename, overwrite=overwrite, checksum=True) |
There was a problem hiding this comment.
Astropy doesn't check this automatically on read but figured it's time we start putting checksums in.
We have been saving just the integers in the mask planes without storing the definitions of the bit positions. Because these definitions are global, reading them later totally messes up the meaning of the saved values.
arunkannawadi
force-pushed
the
tickets/DM-52724
branch
from
cb9946c to
f74073c
Compare
2 participants
Checklist
doc/changespython/lsst/cell_coadds/_fits.pywas modified)