DM-24926: Attempt to get footprints from afw diaSourceCatalog and automatically set the alert cutout size. #84
Conversation
| @@ -715,6 +715,9 @@ def make_dia_source_schema(): | |||
| 'observed in.') | |||
| schema.addField("isDipole", type='Flag', | |||
| doc='Object determined to be a dipole.') | |||
| schema.addField("bboxSize", type='L', | |||
| doc='Length of the square bounding box for cutouts in the ' | |||
| 'alerts.') | |||
There was a problem hiding this comment.
The comments/docstrings above say this is the “minimal schema required for a DIASource”, and that it was “Generated automatically from apdb-schema.yaml in dax_apdb/data”.
I'm pretty sure that adding bboxSize renders at least the second of those comments untrue.
I'm surprised that the bbox size is part of the minimal schema, too, but I should understand the rest of the PR before worrying about that.
There was a problem hiding this comment.
Ok, having read a bit further, I think this is fine. Specifically, what I've understood is that this schema is not exactly what's mapped to the database, but is just being used to pass information through the DIA pipeline (because we read and discard the footprint early in the process, and need somewhere to keep its dimensions).
I think that's fine, but some updates to the comments about where this schema came from and what it's used for would help!
| footprintBBox = inputRecord.getFootprint().getBBox() | ||
| recX = inputRecord.getCentroid().x | ||
| recY = inputRecord.getCentroid().y | ||
| bboxSize = int( |
There was a problem hiding this comment.
int is going to round down here, right? Is that what we want?
There was a problem hiding this comment.
Directly below this line is a ceil call that rounds up no matter what. The int is here only to change the type after the calculation.
| from lsst.utils import getPackageDir | ||
| import lsst.utils.tests | ||
|
|
||
|
|
||
| def make_input_source_catalog(n_objects, add_flags=False): | ||
| def make_input_source_catalog(dataset, add_flags=False): |
There was a problem hiding this comment.
Docstring needs updating to match this change.
| import lsst.afw.image as afwImage | ||
| import lsst.afw.image.utils as afwImageUtils | ||
| import lsst.geom as geom | ||
| import lsst.meas.base.tests as measTests |
There was a problem hiding this comment.
Should add meas_base as setupRequired to ups/ap_association.table.
| @@ -181,6 +178,26 @@ def _patchDiaObjects(self, diaObjects): | |||
| diaObjects["zLcNonPeriodic"] = None | |||
| diaObjects["yLcNonPeriodic"] = None | |||
|
|
|||
| def createDiaSourceBBox(self, bboxSize): | |||
| """Compute the size of the bounding box for this diaSource. | |||
There was a problem hiding this comment.
It'd be worth clarifying this docstring a bit — it's kinda surprising that it takes an argument called bboxSize and yet it “computes the size of the bounding box”.
Perhaps something like “given the maximum dimension of the source footprint, in pixels, return an appropriate bounding box”?
| else: | ||
| obj.set(subSchema.getKey(), 1) | ||
| return objects | ||
| if subSchema.getField().getName() == "ip_diffim_DipoleFit_neg_instFlux": |
There was a problem hiding this comment.
I realise the logic here is mostly not new, just rearranged, but... I kinda feel like a comment about what's going on would be helpful. Why are these particular fields being set to these particular values?
|
Hey @jdswinbank, I'm ready for you have a look again. |
Additionally create unittests for new method.
Debug unittest. Fix linting and add bbox test to run.
Debug unittest.
Reword docstring in DiaSource schema. Edit docstring in unittest.
Additionally alphabetize packages in ups table. Add clarifying comment to fake diaSource creation.
morriscb
force-pushed
the
tickets/DM-24926
branch
from
842ace8
to
4016ad3
Compare
No description provided.