DM-54275: Filter diaSources based on bbox when trailed sources unavailable #362
Conversation
mrawls
left a comment
mrawls
left a comment
There was a problem hiding this comment.
I'm convinced this does what you want, but it's necessary to get pretty deep in the weeds to figure out what it is doing. My main suggestion is to please add a detailed class docstring that explains all the various ways diaSources are filtered and what the resulting flag situation looks like in each case.
| rejected_mask |= trail_mask | ||
| diaSourceCat = diaSourceCat[~rejected_mask].copy(deep=True) | ||
|
|
||
| self.log.info("%i DiaSources exceed max_trail_length %f arcseconds per second, " |
There was a problem hiding this comment.
Suggest breaking this into 2 log messages that only print when num_X_filtered > 0.
In addition,
Suggested change
self.log.info("%i DiaSources exceed max_trail_length %f arcseconds per second, "
self.log.info("%i DiaSources exceed max_trail_length (%f arcseconds per second), "
| self.log.info("%i DiaSources exceed max_trail_length %f arcseconds per second, " | ||
| "dropping from source catalog. %i DiaSources had no" | ||
| "trail calculation and their bounding box exceeded" | ||
| "%f arcseconds in either direction or across the diagonal," |
There was a problem hiding this comment.
| "%f arcseconds in either direction or across the diagonal," | |
| "max_trail_length (%f arcseconds per second) in either direction or across the diagonal," |
| """Check bounding boxes of DiaSources with failed trail measurements. | ||
|
|
||
| For sources where the trail measurement flag is set (indicating the | ||
| trail length is unreliable), fall back to checking the footprint |
There was a problem hiding this comment.
Unreliable or unable to be measured, or both?
There was a problem hiding this comment.
I meant to type unavailable, but I think swapping it to unable to be measured is the most clear.
There was a problem hiding this comment.
That makes more sense - yes, I like "unable to be measured" best. I don't think you've implemented that change yet.
| Returns | ||
| ------- | ||
| bbox_mask : `numpy.ndarray` | ||
| Boolean mask for diaSources whose bounding box width, height, |
There was a problem hiding this comment.
| Boolean mask for diaSources whose bounding box width, height, | |
| Boolean mask for diaSources whose footprint bounding box width, height, |
There was a problem hiding this comment.
Now that this Task is getting a bit complicated, it's definitely time to please add a full class docstring. See https://developer.lsst.io/python/numpydoc.html#documenting-classes
| """Test sources with ext_trailedSources_Naive_flag set but a small bounding | ||
| box should are not flagged. |
There was a problem hiding this comment.
| """Test sources with ext_trailedSources_Naive_flag set but a small bounding | |
| box should are not flagged. | |
| """Test that sources with ext_trailedSources_Naive_flag set but with a small bounding | |
| box are not flagged. |
| # threshold for max_trail_length * exposure_time / pixelScale | ||
| self.assertFalse(bbox_mask[0]) | ||
|
|
||
| def test_check_dia_source_trail_bbox_flag_large_bbox(self): |
There was a problem hiding this comment.
Is it ever possible to have ext_trailedSources_Naive_flag not set and to have a bbox measured? (asking as a possible other unit test to run)
There was a problem hiding this comment.
I added one more unit check that checks that sources with no flag but large bboxes do not get filtered
|
|
||
| if self.config.doTrailedSourceFilter: | ||
| trail_mask = self._check_dia_source_trail(diaSourceCat, exposure_time) | ||
| bbox_mask = self._check_dia_source_trail_bbox(diaSourceCat, exposure_time) |
There was a problem hiding this comment.
Would it be possible to only run this if the initial trail measurement fails? It looks like you calculate it every time, but perhaps there's a reason for that.
There was a problem hiding this comment.
We do, but only in the function. Doing that here would maybe be a bit messy because I'm sending the whole source catalog. Instead I've done an initial filter inside the function so that it doesn't bother checking each source individually. I probably should have done it that way to begin with for speed.
bsmartradio
force-pushed
the
tickets/DM-54275
branch
2 times, most recently
from
31cb6c2 to
496f312
Compare
| """Filter sources from the supplied DiaSource catalog. | ||
|
|
||
| Sources are filtered based on their trail length, sky sources, and bad | ||
| flags. Aby source which meets the filtering criteria is removed from |
There was a problem hiding this comment.
There's a typo (Aby --> Any) and also some odd indentation happening here.
Suggested rephrase: "DiaSources are filtered based on criteria including trail length, whether they are a sky source, and whether they have one or more bad flags. Any source which meets a filtering criteria is removed from the main output catalog and optionally saved in one or more separate output catalogs."
|
It looks like you've resolved most of the review comments, thank you. There are a couple final minor items. Please address those, rebase your commits, run pep8/black if you haven't (a few funky formatting things seem to exist and it doesn't appear auto-enforced on this repo 😬), and summarize what you did on the Jira ticket. |
Code and unit tests are Claude assisted.
bsmartradio
force-pushed
the
tickets/DM-54275
branch
from
496f312 to
990b4ce
Compare
2 participants
No description provided.