More post-review cleanup

Make minimize() return an enum type

explicit delete or default copy/move for star classes

include/lsst/jointcal/CcdImage.h

@@ -68,7 +68,7 @@ class CcdImage {
                      std::string const &fluxField);
 
 public:
-    CcdImage(lsst::afw::table::SourceCatalog &record, std::shared_ptr<lsst::afw::image::TanWcs> wcs,
+    CcdImage(afw::table::SourceCatalog &record, std::shared_ptr<lsst::afw::image::TanWcs> wcs,
              std::shared_ptr<lsst::afw::image::VisitInfo> visitInfo, afw::geom::Box2I const &bbox,
              std::string const &filter, std::shared_ptr<afw::image::PhotoCalib> photoCalib, int visit,
              int ccd, std::string const &fluxField);

include/lsst/jointcal/ConstrainedPhotometryModel.h

@@ -42,7 +42,7 @@ class ConstrainedPhotometryModel : public PhotometryModel {
                                      Eigen::VectorXd &derivatives) override {}
 
 private:
-    PhotometryMapping *findMapping(CcdImage const &ccdImage, std::string name) const {
+    PhotometryMapping *findMapping(CcdImage const &ccdImage, std::string name) const override {
         return nullptr;  // waiting on creation of ConstrainedPhotometryModel.cc
         // auto i = _myMap.find(&ccdImage);
         // if (i == _myMap.end())

include/lsst/jointcal/FittedStar.h

@@ -37,7 +37,6 @@ struct PmBlock {
 class FittedStar : public BaseStar, public PmBlock {
 private:
     double _mag;
-    double _emag;
     int _gen;
     double _wmag;
     unsigned _indexInMatrix;
@@ -50,7 +49,6 @@ class FittedStar : public BaseStar, public PmBlock {
     FittedStar()
             : BaseStar(),
               _mag(-1),
-              _emag(-1),
               _gen(-1),
               _wmag(0),
               _indexInMatrix(-1),
@@ -60,7 +58,6 @@ class FittedStar : public BaseStar, public PmBlock {
     FittedStar(const BaseStar& baseStar)
             : BaseStar(baseStar),
               _mag(-1),
-              _emag(-1),
               _gen(-1),
               _wmag(0),
               _indexInMatrix(0),
@@ -70,6 +67,13 @@ class FittedStar : public BaseStar, public PmBlock {
     //!
     FittedStar(const MeasuredStar& measuredStar);
 
+    /// No move, allow copy constructor: we may copy the fitted StarLists when associating and matching
+    /// catalogs, otherwise Stars should be managed by shared_ptr only.
+    FittedStar(FittedStar const&) = default;
+    FittedStar(FittedStar&&) = delete;
+    FittedStar& operator=(FittedStar const&) = delete;
+    FittedStar& operator=(FittedStar&&) = delete;
+
     //!
     void clearBeforeAssoc() {
         _indexInMatrix = -1;

include/lsst/jointcal/FitterBase.h

@@ -12,6 +12,13 @@
 namespace lsst {
 namespace jointcal {
 
+/// Return value of minimize()
+enum class MinimizeResult {
+    Converged,      // fit has converged - no more outliers
+    Chi2Increased,  // still some ouliers but chi2 increases
+    Failed          // factorization failed
+};
+
 /**
  * Base class for fitters.
  *
@@ -45,18 +52,15 @@ class FitterBase {
      * @param[in]  nSigmaCut  How many sigma to reject outliers at. Outlier
      *                        rejection ignored for nSigmaCut=0.
      *
-     * @return     Return code describing success of fit, can take 3 values:
-     *             0 : fit has converged - no more outliers
-     *             1 : still some ouliers but chi2 increased
-     *             2 : factorization failed
+     * @return  Return code describing success/failure of fit.
      *
      * @note   When fitting one parameter set by itself (e.g. "Model"), the system is purely linear,
      *         which should result in the optimal chi2 after a single step. This can
      *         be used to debug the fitter by fitting that parameter set twice in a row:
      *         the second run with the same "whatToFit" will produce no change in
      *         the fitted parameters, if the calculations and indices are defined correctly.
      */
-    int minimize(std::string const &whatToFit, double nSigmaCut = 0);
+    MinimizeResult minimize(std::string const &whatToFit, double nSigmaCut = 0);
 
     /**
      * Returns the chi2 for the current state.

include/lsst/jointcal/MeasuredStar.h

@@ -47,6 +47,13 @@ class MeasuredStar : public BaseStar {
     MeasuredStar(BaseStar const &baseStar)
             : BaseStar(baseStar), mag(0.), wmag(0.), _id(0), _instFluxErr(0.), _ccdImage(0), _valid(true) {}
 
+    /// No move, allow copy constructor: we may copy the fitted StarLists when associating and matching
+    /// catalogs, otherwise Stars should be managed by shared_ptr only.
+    MeasuredStar(MeasuredStar const &) = default;
+    MeasuredStar(MeasuredStar &&) = delete;
+    MeasuredStar &operator=(MeasuredStar const &) = delete;
+    MeasuredStar &operator=(MeasuredStar &&) = delete;
+
     void setFittedStar(std::shared_ptr<FittedStar> fittedStar) {
         if (fittedStar) fittedStar->getMeasurementCount()++;
         _fittedStar = std::move(fittedStar);

include/lsst/jointcal/PhotometryTransfo.h

@@ -28,13 +28,9 @@ class PhotometryTransfoSpatiallyInvariant;
 class PhotometryTransfo {
 public:
     /// Apply the transform to instFlux at (x,y), put result in flux
-    virtual void apply(double x, double y, double instFlux, double &flux) const = 0;
+    virtual double apply(double x, double y, double instFlux) const = 0;
 
-    double apply(Point const &in, double instFlux) const {
-        double flux;
-        apply(in.x, in.y, instFlux, flux);
-        return flux;
-    }
+    double apply(Point const &in, double instFlux) const { return apply(in.x, in.y, instFlux); }
 
     /// dumps the transfo coefficients to stream.
     virtual void dump(std::ostream &stream = std::cout) const = 0;
@@ -71,7 +67,7 @@ class PhotometryTransfoSpatiallyInvariant : public PhotometryTransfo {
 public:
     PhotometryTransfoSpatiallyInvariant(double value = 1) : _value(value) {}
 
-    void apply(double x, double y, double instFlux, double &out) const override { out = instFlux * _value; }
+    double apply(double x, double y, double instFlux) const override { return instFlux * _value; }
 
     void dump(std::ostream &stream = std::cout) const override { stream << _value; }
 

include/lsst/jointcal/RefStar.h

@@ -25,8 +25,12 @@ class RefStar : public BaseStar {
             : BaseStar(xx, yy, defaultFlux, defaultFluxErr),
               _refFluxList(refFluxList),
               _refFluxErrList(refFluxErrList) {}
-    //!
-    RefStar(const BaseStar& baseStar) : BaseStar(baseStar) {}
+
+    /// No move or copy: each RefStar is unique, and should be accessed/managed via shared_ptr.
+    RefStar(RefStar const&) = delete;
+    RefStar(RefStar&&) = delete;
+    RefStar& operator=(RefStar const&) = default;
+    RefStar& operator=(RefStar&&) = delete;
 
     void dump(std::ostream& stream = std::cout) const {
         BaseStar::dump(stream);

include/lsst/jointcal/SimplePhotometryModel.h

@@ -69,7 +69,7 @@ class SimplePhotometryModel : public PhotometryModel {
     MapType _myMap;
 
     /// Return the mapping associated with this ccdImage. name is a descriptor for error messages.
-    PhotometryMapping *findMapping(CcdImage const &ccdImage, std::string name) const;
+    PhotometryMapping *findMapping(CcdImage const &ccdImage, std::string name) const override;
 };
 
 }  // namespace jointcal

python/lsst/jointcal/fitter.cc

@@ -65,6 +65,11 @@ PYBIND11_PLUGIN(fitter) {
     py::module::import("lsst.jointcal.photometryModels");
     py::module mod("fitter");
 
+    py::enum_<MinimizeResult>(mod, "MinimizeResult")
+            .value("Converged", MinimizeResult::Converged)
+            .value("Chi2Increased", MinimizeResult::Chi2Increased)
+            .value("Failed", MinimizeResult::Failed);
+
     declareFitterBase(mod);
     declareAstrometryFit(mod);
     declarePhotometryFit(mod);

python/lsst/jointcal/jointcal.py

@@ -22,6 +22,7 @@
 from .dataIds import PerTractCcdDataIdContainer
 
 import lsst.jointcal
+from lsst.jointcal import MinimizeResult
 
 __all__ = ["JointcalConfig", "JointcalTask"]
 
@@ -536,18 +537,18 @@ def _iterate_fit(self, fit, max_steps, name, whatToFit):
             r = fit.minimize(whatToFit, 5)  # outlier removal at 5 sigma.
             chi2 = fit.computeChi2()
             self.log.info(str(chi2))
-            if r == 0:
+            if r == MinimizeResult.Converged:
                 self.log.debug("fit has converged - no more outliers - redo minimixation"
                                "one more time in case we have lost accuracy in rank update")
                 # Redo minimization one more time in case we have lost accuracy in rank update
                 r = fit.minimize(whatToFit, 5)  # outliers removal at 5 sigma.
                 chi2 = fit.computeChi2()
                 self.log.info("Fit completed with: %s", str(chi2))
                 break
-            elif r == 2:
+            elif r == MinimizeResult.Failed:
                 self.log.warn("minimization failed")
                 break
-            elif r == 1:
+            elif r == MinimizeResult.Chi2Increased:
                 self.log.warn("still some ouliers but chi2 increases - retry")
             else:
                 self.log.error("unxepected return code from minimize")

python/lsst/jointcal/star.cc

@@ -64,7 +64,8 @@ void declareBaseStar(py::module &mod) {
 void declareRefStar(py::module &mod) {
     py::class_<RefStar, std::shared_ptr<RefStar>, BaseStar> cls(mod, "RefStar");
 
-    cls.def(py::init<BaseStar const &>(), "baseStar"_a);
+    cls.def(py::init<double, double, double, double, std::vector<double> &, std::vector<double> &>(), "xx"_a,
+            "yy"_a, "defaultFlux"_a, "defaultFluxErr"_a, "refFluxList"_a, "refFluxErrList"_a);
 }
 
 void declareFittedStar(py::module &mod) {

src/CcdImage.cc

@@ -75,8 +75,7 @@ void CcdImage::LoadCatalog(afw::table::SourceCatalog const &catalog, std::string
     _wholeCatalog.setCcdImage(this);
 }
 
-CcdImage::CcdImage(lsst::afw::table::SortedCatalogT<lsst::afw::table::SourceRecord> &catalog,
-                   std::shared_ptr<lsst::afw::image::TanWcs> wcs,
+CcdImage::CcdImage(afw::table::SourceCatalog &catalog, std::shared_ptr<lsst::afw::image::TanWcs> wcs,
                    std::shared_ptr<lsst::afw::image::VisitInfo> visitInfo, afw::geom::Box2I const &bbox,
                    std::string const &filter, std::shared_ptr<afw::image::PhotoCalib> photoCalib, int visit,
                    int ccdId, std::string const &fluxField)

src/FittedStar.cc

@@ -19,7 +19,6 @@ namespace jointcal {
 FittedStar::FittedStar(const MeasuredStar &measuredStar)
         : BaseStar(measuredStar),
           _mag(measuredStar.getMag()),
-          _emag(-1),
           _gen(-1),
           _wmag(measuredStar.getMagWeight()),
           _indexInMatrix(-1),

src/FitterBase.cc

@@ -108,14 +108,10 @@ unsigned FitterBase::findOutliers(double nSigmaCut, MeasuredStarList &msOutliers
     return nOutliers;
 }
 
-int FitterBase::minimize(std::string const &whatToFit, double nSigmaCut) {
+MinimizeResult FitterBase::minimize(std::string const &whatToFit, double nSigmaCut) {
     assignIndices(whatToFit);
 
-    // return code can take 3 values :
-    // 0 : fit has converged - no more outliers
-    // 1 : still some ouliers but chi2 increases
-    // 2 : factorization failed
-    int returnCode = 0;
+    MinimizeResult returnCode = MinimizeResult::Converged;
 
     // TODO : write a guesser for the number of triplets
     unsigned nTrip = (_lastNTrip) ? _lastNTrip : 1e6;
@@ -145,7 +141,7 @@ int FitterBase::minimize(std::string const &whatToFit, double nSigmaCut) {
     CholmodSimplicialLDLT2<SpMat> chol(hessian);
     if (chol.info() != Eigen::Success) {
         LOGLS_ERROR(_log, "minimize: factorization failed ");
-        return 2;
+        return MinimizeResult::Failed;
     }
 
     unsigned totalOutliers = 0;
@@ -158,7 +154,7 @@ int FitterBase::minimize(std::string const &whatToFit, double nSigmaCut) {
         LOGLS_DEBUG(_log, currentChi2);
         if (currentChi2.chi2 > oldChi2) {
             LOGL_WARN(_log, "chi2 went up, skipping outlier rejection loop");
-            returnCode = 1;
+            returnCode = MinimizeResult::Chi2Increased;
             break;
         }
         oldChi2 = currentChi2.chi2;
@@ -169,8 +165,8 @@ int FitterBase::minimize(std::string const &whatToFit, double nSigmaCut) {
         int nOutliers = findOutliers(nSigmaCut, msOutliers, fsOutliers);
         totalOutliers += nOutliers;
         if (nOutliers == 0) break;
-        TripletList tripletList(1000);  // initial allocation size.
-        grad.setZero();                 // recycle the gradient
+        TripletList tripletList(nOutliers);
+        grad.setZero();  // recycle the gradient
         // compute the contributions of outliers to derivatives
         outliersContributions(msOutliers, fsOutliers, tripletList, grad);
         // Remove significant outliers