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-11023: CoaddPsf: add more exception info #83

Merged
merged 2 commits into from Sep 25, 2017
Merged

Conversation

PaulPrice
Copy link
Contributor

A common problem when calling CoaddPsf.computePsfImage() is that
WarpedPsf.computeBBoxFromTransform throws a RangeError stating
'Unexpectedly large transform passed to WarpedPsf'. This is
usually because the astrometry for an input is poor, but it would
be nice to identify which input: so add the exposure ID to the
exception message.

@SimonKrughoff
Copy link
Contributor

Two things.
First thing, is it the right thing to do to Except here? I feel like it would be o.k. to report the failure (with dataId) drop it and continue on. The current system makes astrometry failures separated greatly from this piece of code completely halt the processing. It would be nice to be able to continue on even if a few failures are encountered and fix up the failures if necessary. What do you think?
Second thing, is this code covered by a unit test? It would be nice if it was.

@PaulPrice
Copy link
Contributor Author

I don't see how it's possible to continue on. We could ignore the error and not include that image's Psf in the CoaddPsf, but that means the CoaddPsf would be wrong. Or we could remove the test in WarpedPsf.computeBBoxFromTransform that's throwing the exception in the first place, but that means we're going to have to get even further along before discovering something's bad. I think what we need is better quality control on the astrometry (as you suggest, identify problems close to where the problem occurs), and I understand that's something that's being actively worked on so it's not something I intend to tackle here.
Regarding a unit test, I would prefer to avoid testing the contents of an exception message, as it's something that's not so important and can change easily (line numbers will change, additional information may be added) --- the same reasons that we don't test the exact contents of log outputs.

@SimonKrughoff
Copy link
Contributor

O.K. fair point with regard to discovering astrometric problems early.
Regarding, the unit test, I was not asking to test the content of the exception, just that an exception gets thrown. I'll leave it up to you, but it would be nice if this code were at least called in a unit test somewhere.

Copy link
Contributor

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

See comments in the general PR discussion.

@PaulPrice
Copy link
Contributor Author

OK, I just checked and I don't see an existing test of the exception. Adding one is more work than I originally signed up for, but I'll see if I can make one quickly.

@PaulPrice
Copy link
Contributor Author

Added a test, and rebased against master.

A common problem when calling CoaddPsf.computePsfImage() is that
WarpedPsf.computeBBoxFromTransform throws a RangeError stating
'Unexpectedly large transform passed to WarpedPsf'. This is
usually because the astrometry for an input is poor, but it would
be nice to identify *which* input: so add the exposure ID to the
exception message.
CoaddPsf throws a RangeError when one of the inputs has an unexpectedly
large transform. This tests that behaviour.
@PaulPrice PaulPrice merged commit 9c721e1 into master Sep 25, 2017
@ktlim ktlim deleted the tickets/DM-11023 branch August 25, 2018 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants