DM-10826: fix jointcal collectRefStar handling of position errors #136
Conversation
parejkoj
force-pushed
the
tickets/DM-10826
branch
from
5adea1d
to
c2f0a78
Compare
python/lsst/jointcal/jointcal.py
Outdated
| # RuntimeError also feels too generic, but may be better? | ||
| ##################### | ||
| raise KeyError("Reference catalog does not contain coordinate errors, and" | ||
| "config.astrometryReferenceErr not supplied.") |
There was a problem hiding this comment.
Yeah, I see your Error dilemma. The second half is a KeyError, but 99% of the time this is going to come up when someone forgets to set the config.astrometryReferenceErr, so I'd argue the first half is more important. So logically this is a forgotten config parameter that the user needs to supply. I've used ValueError for misconfigurations in the past: https://github.com/lsst/pipe_tasks/blob/master/python/lsst/pipe/tasks/assembleCoadd.py#L210
There was a problem hiding this comment.
I think this probably boils down to making this exception useful on a catch side. If you want your caller/catcher to distinguish between different exception types then it may be better to define special exception class for it. If you are not sure that it will ever be caught then exception type probably does not matter at all, but message itself should be very clear and unambiguous.
There was a problem hiding this comment.
@yalsayyad : I think the case you cite there in AssembleCoaddConfig should be using pexConfig.FieldValidationError instead, since it's part of the Config validation. Which, as I think about it, is probably the correct thing to raise here too, since as you say in this case the user probably should have supplied it.
To the "what happens if it's caught" point, I think this is a non-recoverable error that the user will have to sort out, which FieldValidationError should make pretty clear.
There was a problem hiding this comment.
Having done that, that's definitely the right error to raise. The resulting message has a lot of additionally useful information:
E lsst.pex.config.config.FieldValidationError: Field 'astrometryReferenceErr' failed validation: Reference catalog does not contain coordinate errors, and config.astrometryReferenceErr not supplied.
E For more information read the Field definition at:
E File jointcal/jointcal.py:299 (JointcalConfig)And the Config definition at:
E File jointcal/jointcal.py:135 (<module>)
| @@ -671,6 +686,17 @@ def _load_reference_catalog(self, refObjLoader, referenceSelector, center, radiu | |||
| else: | |||
| refCat = selected.sourceCat | |||
|
|
|||
| if self.config.astrometryReferenceErr is None and 'coord_ra_err' not in refCat.schema: | |||
There was a problem hiding this comment.
The scenario that I'm worried about is the HSC processing which uses our PS1 refcat, which has coord_ra_err and coord_dec_err, but they're all populated with zeros. I'm not sure where's the best place to check that. Maybe it doesn't matter because the fit will fail pretty quickly if its using zero errors (I'll test this after typing this), and the user will remember to add an astrometryReferenceErr?
There was a problem hiding this comment.
I'm not sure how much we should do to protect users from clearly incorrect refcats. I could add a check that vx >= 0 && vy >= 0 reject that reference source in that case? This would would result in there being no reference stars at all for the fit. I'm not sure what exactly would happen there, but I suspect it wouldn't be good; there would be a lot of log warnings about not enough refstars per sensor.
Adding an explicit check for the number of cross matches is DM-6354.
There was a problem hiding this comment.
I'm not referring to a hypothetical refcat, I'm talking about our current defaults This is going to crash with the current default configs:
jointcal INFO: Applying color terms for filterName='g' reference catalog=ps1_pv3_3pi_20170110
jointcal FATAL: Failed processing tract 9615, TypeError:
File "src/table/Schema.cc", line 211, in lsst::afw::table::SchemaItem<T> lsst::afw::table::detail::SchemaImpl::find(const string&) const [with T = double; std::string = std::basic_string<char>]
Field 'coord_ra_err' does not have the given type. {0}
lsst::pex::exceptions::TypeError: 'Field 'coord_ra_err' does not have the given type.'```
There was a problem hiding this comment.
Weird. Can you please give me a to-reproduce command? It looks like the error is coming from when it tries to do something with that field.
There was a problem hiding this comment.
Thanks for the to-reproduce on the ticket branch. The crash was because I was looking up a Key<double> for the coordinate errors (they were Key<float>). I've fixed that problem. Now, with the default config, your HSC command fails during fit initialization because the chi2 is NaN (as I would expect, with 0 refcat coordinate errors).
Adding --config astrometryReferenceErr=10 let it run to completion. We just need to decide whether the correct thing is to make that the default (given that we don't have refcat coord errors right now), or to leave the default be "assume the refcat has errors" and override the above for HSC until I can finish prepping the Gaia DR2 refcat.
There was a problem hiding this comment.
If you'd rather put it in the obs_packages, that'd be fine with me. obs_decam and obs_subaru would get the --config astrometryReferenceErr=10 and obs_lsst/imsim can get a --config astrometryReferenceErr=1, obs_cfht didn't have a jointcal.py and I didn't look beyond that.
I tested defaults with obs_lsst and got your nice error jointcal.astrometryReferenceSelector INFO: Selected 1460063/1460063 references jointcal FATAL: Failed processing tract 3633, FieldValidationError: Field 'astrometryReferenceErr' failed validation: Reference catalog does not contain coordinate errors, and config.astrometryReferenceErr not supplied. For more information see the Field definition at: File jointcal/jointcal.py:299 (JointcalConfig) and the Config definition at: File jointcal/jointcal.py:135 (<module>)
Re: 'ps1_pv3_3pi_20170110', even though a chi=NaN is expected, I'm not sure if someone who wanted to try jointcal with says CFHT data and with 'ps1_pv3_3pi_20170110' ref_cat, would know that the solution to the failed fit initialization would be to add an 'astrometryReferenceErr' config. If you argue, "they should look in obs_subaru and obs_decam," I'll let it go.
There was a problem hiding this comment.
Unfortunately, we don't have a place to put config options that go with a reference catalog (instead of an obs package). I think our best bet for now is to put that config in the obs packages, but I'm not really happy about either option (defaulting to a non-NaN astrometryReferenceErr means that the better refcats will require changing the value).
python/lsst/jointcal/jointcal.py
Outdated
| # RuntimeError also feels too generic, but may be better? | ||
| ##################### | ||
| raise KeyError("Reference catalog does not contain coordinate errors, and" | ||
| "config.astrometryReferenceErr not supplied.") |
There was a problem hiding this comment.
I think this probably boils down to making this exception useful on a catch side. If you want your caller/catcher to distinguish between different exception types then it may be better to define special exception class for it. If you are not sure that it will ever be caught then exception type probably does not matter at all, but message itself should be very clear and unambiguous.
parejkoj
force-pushed
the
tickets/DM-10826
branch
from
2573e69
to
c6f59b3
Compare
|
Thank you both for your suggestions. I've incorporated everything that I could, and left a couple of open questions in the threads above. |
|
@andy-slac @yalsayyad : do you have any responses to my above unresolved comments, or further thoughts? |
|
I'm totally OK with proposed |
|
I think I have approved this PR, but github only says that I reviewed it. I'm fine with whatever you decide to do in response to my comments. It's also OK to mark JIRA ticket as reviewed once @yalsayyad feels things are corrected. |
This allows us to change the reference catalog coordinate errors if they are not specified in the refcat itself. NOTE: Unfortunately, none of the current testdata_jointcal refcats have coordinate errors, so we don't have proper tests of the code that uses the reference catalog errors.
No description provided.