DM-22777: Prune diaForcedSources in DiaForcedSourceTask #71
Conversation
| @@ -264,3 +266,31 @@ def _calibrate_and_merge(self, | |||
| output_catalog.drop(columns=self.config.dropColumns, inplace=True) | |||
|
|
|||
| return output_catalog | |||
|
|
|||
| def _trim_to_bbox(self, catalog, exposure): | |||
There was a problem hiding this comment.
It seems a bit odd to have a trim_to_bbox method that takes an exposure. Why not let it take a Box2I (more reusable) or call it trim_to_exposure (more robust to changes in algorithm, pixel vs. celestial coordinates, etc)?
There was a problem hiding this comment.
Changed to trim_to_exposure
| return catalog[np.logical_and(np.logical_and(minX < xS, maxX > xS), | ||
| np.logical_and(minY < yS, maxY > yS))] |
There was a problem hiding this comment.
Box2D has a vector contains method, so I think you can simplify this to something like:
return catalog[bbox.contains(xS, yS)]
The loop over xS and yS is done in C++, so this should also be faster than the current code.
There was a problem hiding this comment.
Thanks, didn't see the vectorized bbox.contains.
| src = objects.addNew() | ||
| src['id'] = 10000000 | ||
| src.setCoord(wcs.pixelToSky(-100000, | ||
| -100000)) |
There was a problem hiding this comment.
Actually, one more suggestion: consider testing points that are out-of-bounds in only X or only Y, or some that are just out-of-bounds (especially points that would be treated differently by Box2I vs. Box2D).
Rename trim_to_bbox to trim_to_exposure. Directly use Box2D.contains Add two more test points outside of ccd.
morriscb
force-pushed
the
tickets/DM-22777
branch
from
8fa5ed3
to
68d9fe3
Compare
No description provided.