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-38826: ZeroDivisionError in lsst.cp.pipe.defects.MeasureDefectsCombinedWithFilterTask #184

Merged
merged 2 commits into from Apr 26, 2023

Conversation

plazas
Copy link
Contributor

@plazas plazas commented Apr 25, 2023

No description provided.

@plazas plazas requested a review from erykoff April 25, 2023 16:46
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

Can you clarify if it's finding all defect or no defect?

y = self.defaultConfig.nPixBorderLeftRight
shapeY, shapeX = exp.image.array.shape
width, height = shapeX - 2*x, shapeY - 2*x
shouldBeFound = [Box2I(corner=Point2I(x, y), dimensions=Extent2I(width, height))]
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is supposed to find all defects in this case? Shouldn't it find none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out empirically by running this test that it is declaring the whole detector as a defect (one box). I could only do it perhaps for just one amp?

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 we want it to mask out one amp, and then I think it should return no defects, at least through this code path. Can you figure out where it's marking everything as a defect?

Copy link
Contributor Author

@plazas plazas Apr 25, 2023

Choose a reason for hiding this comment

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

I think that's the footprint that the afw code returns with that sigma:

ipdb> footprintList [995488 peaks, area=1000486, centroid=(257.5, 2996)] ipdb> defects = Defects.fromFootprintList(footprintList) ipdb> defects.toDict() {'metadata': <lsst.daf.base.propertyContainer.propertyList.PropertyList object at 0x7f91b8103530>, 'x0': [7], 'y0': [2000], 'width': [502], 'height': [1993]}

The above is for amp C10 in the example that Jim paste din the ticket.

As for the unit test, I'll do it only for one amp.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, so that's not from afw, that's from the code that explicitly makes a bad amp if sigma=0.0. And that is currently the intended behavior, but we may revisit it. So this is actually fine as it is.

@@ -650,8 +657,10 @@ def valueThreshold(self, fileType):
self.assertIn(expectedBBox, boxesMeasured)

def test_valueThreshold(self):
for fileType in ['flat', 'flat']:
for fileType in ['dark', 'flat']:
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this!

plazas added 2 commits April 25, 2023 14:36
Saturate an amp instead of the whole flat in test.
@plazas plazas merged commit 797d208 into main Apr 26, 2023
1 check passed
@plazas plazas deleted the tickets/DM-38826 branch April 26, 2023 00:35
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.

None yet

2 participants