Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-30993: Implement reference catalog culling for astrometry in SFM #410

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

laurenam
Copy link
Contributor

@laurenam laurenam commented Feb 3, 2025

No description provided.

Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a concern about how coordinates are handled in the new CullFromMaskedRegion. I have a few different suggestions in the comments, depending on the desired behavior.

Comment on lines 624 to 637
bbox = exposure.getBBox()
xMax = bbox.getMaxX()
yMax = bbox.getMaxY()
# If reference object nominally lies outside the detector, consider
# it to be at the edge (and thus obeys those mask planes).
xRefList = [int(min(max(0, xRef), xMax)) for xRef in xRefList]
yRefList = [int(min(max(0, yRef), yMax)) for yRef in yRefList]
badMaskNames = []
maskPlaneDict = exposure.getMask().getMaskPlaneDict()
for badName in self.badMaskNames:
if badName in maskPlaneDict:
badMaskNames.append(badName)
bitmask = exposure.mask.getPlaneBitMask(badMaskNames)
toKeep = ((exposure.mask.array & bitmask) == 0)
selected = toKeep[yRefList, xRefList] # x & y flipped for numpy arrays
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will break if the bbox has a non-zero XY0.

Possible alternative:

xRefList = catalog[self.xColName]
yRefList = catalog[self.yColName]
bbox = exposure.getBBox()
badMaskNames = []
maskPlaneDict = exposure.getMask().getMaskPlaneDict()
for badName in self.badMaskNames:
    if badName in maskPlaneDict:
        badMaskNames.append(badName)
bitmask = exposure.mask.getPlaneBitMask(badMaskNames)
selected = np.array([(exposure.mask[xv, yv] & bitmask) == 0 for xv, yv in zip(xRefList, yRefList)])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, the above will break if any of the catalog sources are outside the image. That is easy to fix with an extra isInBbox = bbox.contains(xRefList, yRefList) and using that to throw out sources.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternately, to keep the current behavior where off-image sources are moved to the edge of the image for the check, I would suggest:

x0, y0 = exposure.getXY0()
xMax, yMax = exposure.getDimensions()
xRefList = [int(min(max(0, xRef - x0), xMax)) for xRef in xRefList]
yRefList = [int(min(max(0, yRef - y0), yMax)) for yRef in yRefList]

Copy link
Contributor Author

@laurenam laurenam Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATED: my first version was wrong. This is what I have now and I've added what I think works as a test of a non (0, 0) XY0 exposure in the unittest.

Another nice catch, thanks! I do want to keep the current (battle-tested) behavior, so I'm going with this one, but it has to slightly modify it to:

x0, y0 = exposure.getXY0()
xMax, yMax = exposure.getDimensions()
xRefList = [int(min(max(0, xRef - x0), xMax - 1)) for xRef in xRefList]
yRefList = [int(min(max(0, yRef - y0), yMax - 1)) for yRef in yRefList]

Let me know if this looks wrong to you...

@laurenam laurenam force-pushed the tickets/DM-30993 branch 2 times, most recently from ed032ca to 2cba7de Compare February 13, 2025 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants