-
Notifications
You must be signed in to change notification settings - Fork 7
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-43970: Fix unexpected errors in HsmShapeRegauss reported as warnings #70
Conversation
8e330b8
to
55b8de7
Compare
55b8de7
to
b1462fd
Compare
guessCentroid._p, # guess_centroid | ||
hsmparams._hsmp, # hsmparams | ||
) | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need a try
block within a try
block. Just catch both types of errors in the same except
block below. Reference for capturing two types of errors: https://docs.python.org/3/tutorial/errors.html#handling-exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right, that's way better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of wrapping only that one line in another try block was that the RuntimeErrors
were guaranteed to come from GalSim C++. That's still probably going to be true for the rest of the block but not necessarily if someone adds other bits of non-GalSim code to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the code in the block doesn't seem troublesome to me, but I agree with @taranu that keeping them there poses the risk of others adding non-GalSim code to it. Looking at the original C++ code I used for this Python implementation, it actually makes more sense to use one try...except
block and only include GalSim's EstimateShearView
call in this block. This way, catching both GalSimHSMError
and RuntimeError
with this single block addresses Dan's concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. The rest of the block is setup code that should not fail and if it does, it's likely a usage error or something we should catch earlier (i.e. a zero-sized image or something like that), not a GalSim/HSM algorithmic failure.
cb34b49
to
9246087
Compare
@@ -109,7 +109,7 @@ def __init__(self, config, name, schema, metadata, logName=None): | |||
self.hasDeblendKey = len(config.deblendNChild) > 0 | |||
|
|||
if self.hasDeblendKey: | |||
self.deblendKey = schema[config.deblendNChild] | |||
self.deblendKey = schema.find(config.deblendNChild).key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this schema.find
approach considered standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a problem with how it was prior to the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was causing 'lsst.afw.table._table.SubSchema' object has no attribute 'get'
with self.hasDeblendKey==True
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was incorrect before. Either what @enourbakhsh did here, or schema[name].asKey()
is also equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema.find(name).key
method is thanks to Dan's input. If they're both equivalent, I personally find the schema[name].asKey()
method easier to read/digest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One question about GalSim which might help us determine if we need to file other tickets.
@@ -109,7 +109,7 @@ def __init__(self, config, name, schema, metadata, logName=None): | |||
self.hasDeblendKey = len(config.deblendNChild) > 0 | |||
|
|||
if self.hasDeblendKey: | |||
self.deblendKey = schema[config.deblendNChild] | |||
self.deblendKey = schema.find(config.deblendNChild).key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was incorrect before. Either what @enourbakhsh did here, or schema[name].asKey()
is also equivalent.
# GalSim does not raise custom pybind errors as of version 2.5, | ||
# resulting in all GalSim C++ errors being RuntimeErrors. | ||
except (galsim.hsm.GalSimHSMError, RuntimeError) as error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any concern that these RuntimeError's raised by GalSim might be more serious ones that we should further investigate, or can we assume that they're all just internal algorithmic problems and ignore them (which is what this is effectively doing)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The errors were mostly being thrown on objects that were flagged but not ignored due to the bug above, so I'm not too worried about that. They are unfortunately not granular so if you wanted to add more specific flags you'd need to parse the error message.
Note that the comment above mentions DM-42047, which will change the line above to call GalSim's Python interface. But I see that that interface just catches RuntimeError
and re-raises as GalSimHSMError
, which will end up in the same result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree that we can assume these are internal GalSim issues that are expected for some objects and don't need further investigation from us. Additionally, parsing the error message and creating more flags doesn't seem future-proof to me.
f796feb
to
077fcd0
Compare
077fcd0
to
5ae9b9f
Compare
DM-43970: Fix unexpected errors in HsmShapeRegauss reported as warnings
No description provided.