DM-42914: Add edgeCenter and edgeCenterAll flags #264
Conversation
| self.dataset.addSource(100000, lsst.geom.Point2D(80, 30)) | ||
| # Half on the edge; no offimage or edgeCenterAll, but edge and edgeCenter. | ||
| self.dataset.addSource(100000, lsst.geom.Point2D(115, 30)) |
There was a problem hiding this comment.
Does "half on the edge" mean the source sits on the border of the edge region? Is the centroid exactly on the border, or merely <1px off (I can see different actual/desired behavior for the two cases)?
There was a problem hiding this comment.
I've clarified the comments. It's about whether edgeCenter vs. edgeCenterAll is set. All that I care about is where central 3x3 region is, not the exact centroid.
There was a problem hiding this comment.
Yes, but the position of the 3×3 region will depend on where the centroid is, and by how much...
There was a problem hiding this comment.
Do the updated comments help?
There was a problem hiding this comment.
I guess? I'm not sure how the pixels can be "half" on the edge (a pixel is either flagged or not, no?)
There was a problem hiding this comment.
Ah, I meant "half of the" - I'll fix that.
There was a problem hiding this comment.
I meant that I'm not sure how you can get half of them like that -- I would have expected e.g. one row on and two off, or vice versa.
There was a problem hiding this comment.
Pedantism strikes again! You're right, but that doesn't matter for the test itself (nothing cares about the centroid itself, just the box around it).
There was a problem hiding this comment.
How about Central 3x3 pixels are partly on the masked EDGE?
tests/test_PixelFlags.py
Outdated
| np.testing.assert_array_equal(catalog["base_PixelFlags_flag_edgeCenter"], | ||
| np.array([False, True, True, True, True, True, True])) | ||
| np.testing.assert_array_equal(catalog["base_PixelFlags_flag_edgeCenterAll"], | ||
| np.array([False, True, True, True, True, False, True])) |
There was a problem hiding this comment.
This style of testing is very hard to validate (which index is which source? 😵💫), but I suppose that's out of scope for this issue.
There was a problem hiding this comment.
Yeah, I know. I am responsible for adding these tests myself, so I can't blame history on this part. It was useful to have them all in one test so I could look at them on the image with afw.display to check that I'd put them where I thought I had, and then to look at the arrays being tested to see that they covered the cases I wanted.
Suggestions on how to clean it up? I could pull each source into its own test, but then it's a bit harder to reason about having caught an appropriate number of cases.
There was a problem hiding this comment.
Yeah, a separate method for each would probably be best. The "loop over examples" idiom wouldn't really work here, especially if you need explanatory comments for each source. 🙂
There was a problem hiding this comment.
I separated the edge from the offImage tests, so at least there are many fewer bools in the arrays. I think because they're grouped together and only 2 or 4 entries to check, this is clear enough now. Thoughts?
We don't need special handling if we just assign the EDGE table key as the NO_DATA mapping value.
These flags will help distinguish sources that are near an edge vs. those whose center is fully contained in the edge region.
parejkoj
force-pushed
the
tickets/DM-42914
branch
from
04a9dd8
to
200cb05
Compare
parejkoj
force-pushed
the
tickets/DM-42914
branch
from
200cb05
to
0eea8ca
Compare
* We don't need the centered source in all the tests, it just clutters the result when checking bools. * Separate testEdgeFlags from testOffImage
parejkoj
force-pushed
the
tickets/DM-42914
branch
from
0eea8ca
to
dc16ef9
Compare
No description provided.