DM-19768: Fix jointcal handling of coordinate errors #143
Conversation
The astrometry/photometry name passed to _do_load_refcat_and_fit() used to be capitalized, but that changed long ago. Either way, I should have made the comparisons against the name use .lower() to catch both cases. This should fix a few bugs that are not properly exercised by the jointcal tests.
The reference selectors were being set by the caller; these lines were just wrong (it should have been self.X, not self.config.X)!
| referenceSelector = self.config.astrometryReferenceSelector | ||
| elif name == "Photometry": | ||
| referenceSelector = self.config.photometryReferenceSelector | ||
| applyColorterms = False if name.lower() == "astrometry" else self.config.applyColorTerms |
There was a problem hiding this comment.
Should you do a validate on the astrometryReferenceSelector such that its colorLimits is an empty dict (the default) since you are not applying color terms? (also, the magLimit, if used, would be in the native reference catalog filter bandpass, which may either be desired or cause confusion?! ) I think you are relying on the defaults all being set to False in ReferenceSourceSelectorConfig, but these could change from underneath you.
Also, in your validate function at line 324, should you add a and self.doPhotometry clause since you don't need colorTerms if you're not doing photometric calibration. Also, you might want to warn if you're only fitting for astrometry but applyColorTerms is set to True (to let the user know that the fluxes are not being translated into the camera's filter bandpasses...they could conceivably be used within the ReferenceSourceSelectorTask). The default being False for applyColorTerms makes it a mismatch an unlikely scenario at present -- but, again, defaults can get changed.
There was a problem hiding this comment.
So this was just not bombing because of the capitalization in name?! Also, should refObjLoader and referenceSelector both be required parameters to _do_load_refcat_and_fit since having them as None is not really an option. Oh, and I don't see referenceSelector in the docstring's Parameters list.
There was a problem hiding this comment.
I think I'm going to leave mucking around with validate until after some progress has been made on planning the new refcat flux system. I added the self.doPhotometry/self.doAstrometry checks since those didn't require any assumptions about the selector.
Yeah, the mis-capitalization was causing that clause to not trigger, and thus not explode. The person who wrote it (me) was dumb.
Good call on making those required parameters. I've made them required named parameters, using the new python3 syntax for that, and cleaned up the docstrings.
There was a problem hiding this comment.
While I must take serious issue with the 2nd sentence of the 2nd paragraph above, I accept you position on validation... 😉
Review comments suggested clarifications in config validation, since colorterms are only applied for photometry.
No description provided.