post review cleanups

lsst · May 5, 2019 · e9affaf · e9affaf
1 parent d64010e
commit e9affaf
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 22 deletions.
diff --git a/include/lsst/jointcal/Associations.h b/include/lsst/jointcal/Associations.h
@@ -135,15 +135,14 @@ class Associations {
      * @param[in]  matchCut       Separation radius to match fitted and
      *                            reference stars.
      * @param      fluxField      The field name in refCat to get the flux from.
-     * @param      refCoordinateErr Error on reference catalog coordinates (mas). If not NaN, this
+     * @param      refCoordinateErr Error on reference catalog coordinates [mas]. If not NaN, this
      *                              overrides the `coord_*_err` values in the reference catalog itself.
      *                              This value is divided by cos(dec) before being used for ra_err.
      * @param      rejectBadFluxes  Reject reference sources with flux=NaN or 0 and/or fluxErr=NaN or 0.
      *                              Typically false for astrometry and true for photometry.
      */
     void collectRefStars(afw::table::SimpleCatalog &refCat, afw::geom::Angle matchCut,
-                         std::string const &fluxField, float const refCoordinateErr,
-                         bool rejectBadFluxes = false);
+                         std::string const &fluxField, float refCoordinateErr, bool rejectBadFluxes = false);
 
     //! Sends back the fitted stars coordinates on the sky FittedStarsList::inTangentPlaneCoordinates keeps
     //! track of that.

diff --git a/python/lsst/jointcal/jointcal.py b/python/lsst/jointcal/jointcal.py
@@ -133,7 +133,7 @@ def __call__(self, args):
 
 
 class JointcalConfig(pexConfig.Config):
-    """Config for JointcalTask"""
+    """Configuration for JointcalTask"""
 
     doAstrometry = pexConfig.Field(
         doc="Fit astrometry and write the fitted result.",
@@ -291,8 +291,9 @@ class JointcalConfig(pexConfig.Config):
         doc="How to down-select the loaded photometry reference catalog.",
     )
     astrometryReferenceErr = pexConfig.Field(
-        doc="Uncertainty on reference catalog coordinates [mas] if they do not have `coord_*_err` fields."
-            " If None, then raise an exception if the reference catalog is missing coordinate errors.",
+        doc="Uncertainty on reference catalog coordinates [mas] to use in place of the `coord_*_err` fields."
+            " If None, then raise an exception if the reference catalog is missing coordinate errors."
+            " If specified, overrides any existing `coord_*_err` values.",
         dtype=float,
         default=None,
         optional=True
@@ -703,12 +704,12 @@ def _load_reference_catalog(self, refObjLoader, referenceSelector, center, radiu
             refCat = selected.sourceCat
 
         if self.config.astrometryReferenceErr is None and 'coord_ra_err' not in refCat.schema:
-            #####################
-            # TODO: is KeyError really the best thing to raise here?
-            # RuntimeError also feels too generic, but may be better?
-            #####################
-            raise KeyError("Reference catalog does not contain coordinate errors, and"
-                           "config.astrometryReferenceErr not supplied.")
+            msg = ("Reference catalog does not contain coordinate errors, "
+                   "and config.astrometryReferenceErr not supplied.")
+            raise pexConfig.FieldValidationError(JointcalConfig.astrometryReferenceErr,
+                                                 self.config,
+                                                 msg)
+
         if self.config.astrometryReferenceErr is not None and 'coord_ra_err' in refCat.schema:
             self.log.warn("Overriding reference catalog coordinate errors with %f/coordinate [mas]",
                           self.config.astrometryReferenceErr)

diff --git a/src/Associations.cc b/src/Associations.cc
@@ -23,7 +23,6 @@
  */
 
 #include <cmath>
-#include <math.h>  // for isnan
 #include <iostream>
 #include <limits>
 #include <sstream>
@@ -171,7 +170,7 @@ void Associations::associateCatalogs(const double matchCutInArcSec, const bool u
 }
 
 void Associations::collectRefStars(afw::table::SimpleCatalog &refCat, afw::geom::Angle matchCut,
-                                   std::string const &fluxField, float const refCoordinateErr,
+                                   std::string const &fluxField, float refCoordinateErr,
                                    bool rejectBadFluxes) {
     if (refCat.size() == 0) {
         throw(LSST_EXCEPT(pex::exceptions::InvalidParameterError,
@@ -182,7 +181,7 @@ void Associations::collectRefStars(afw::table::SimpleCatalog &refCat, afw::geom:
     // Handle reference catalogs that don't have position errors.
     afw::table::Key<double> raErrKey;
     afw::table::Key<double> decErrKey;
-    if (isnan(refCoordinateErr)) {
+    if (std::isnan(refCoordinateErr)) {
         raErrKey = refCat.getSchema().find<double>("coord_ra_err").key;
         decErrKey = refCat.getSchema().find<double>("coord_dec_err").key;
     }
@@ -214,13 +213,13 @@ void Associations::collectRefStars(afw::table::SimpleCatalog &refCat, afw::geom:
         double dec = lsst::afw::geom::radToDeg(coord.getLatitude());
         auto star = std::make_shared<RefStar>(ra, dec, flux, fluxErr);
 
-        if (isnan(refCoordinateErr)) {
+        if (std::isnan(refCoordinateErr)) {
             star->vx = record->get(raErrKey);
             star->vy = record->get(decErrKey);
         } else {
-            // Add fake errors when the catalog has none
-            star->vx = std::pow(refCoordinateErr / 3600. / cos(coord.getLatitude()), 2);
-            star->vy = std::pow(refCoordinateErr / 3600, 2);
+            // Compute and use the fake errors
+            star->vx = std::pow(refCoordinateErr / 1000. / 3600. / std::cos(coord.getLatitude()), 2);
+            star->vy = std::pow(refCoordinateErr / 1000. / 3600., 2);
         }
         // TODO: cook up a covariance as none of our current refcats have it
         star->vxy = 0.;

diff --git a/tests/config/astrometryReferenceErr-config.py b/tests/config/astrometryReferenceErr-config.py
@@ -1 +1 @@
-config.astrometryReferenceErr = 0.01
+config.astrometryReferenceErr = 10  # milliarcseconds
diff --git a/tests/config/config.py b/tests/config/config.py
@@ -8,4 +8,4 @@
 config.sourceSelector.name = "astrometry"
 config.sourceSelector["astrometry"].sourceFluxType = "Calib"
 config.sourceSelector["astrometry"].badFlags.extend(["slot_Shape_flag", "base_PixelFlags_flag_interpolated"])
-config.astrometryReferenceErr = 0.1  # milli-arcsecond
+config.astrometryReferenceErr = 100  # milli-arcsecond
diff --git a/tests/test_jointcal_cfht.py b/tests/test_jointcal_cfht.py
@@ -27,6 +27,7 @@
 import lsst.afw.geom
 import lsst.utils
 import lsst.pex.exceptions
+import lsst.pex.config
 
 import jointcalTestBase
 
@@ -171,7 +172,7 @@ def test_jointcalTask_2_visits_constrainedAstrometry_astrometryReferenceUncertai
         test_config = os.path.join(lsst.utils.getPackageDir('jointcal'),
                                    'tests/config/astrometryReferenceErr-None-config.py')
         self.configfiles.append(test_config)
-        with self.assertRaises(KeyError):
+        with self.assertRaises(lsst.pex.config.FieldValidationError):
             self._testJointcalTask(2, None, None, None)
 
     def setup_jointcalTask_2_visits_constrainedPhotometry(self):