port jointcal to pybind11 tickets/DM-9187 #31

parejkoj · 2017-03-11T01:10:48Z

No description provided.

Remove swig from sconscript

Cleanup DistortionMap API: produceSipWcs should be in base class.

pschella · 2017-03-15T16:59:53Z

include/lsst/jointcal/MeasuredStar.h

@@ -52,6 +52,12 @@ class MeasuredStar : public BaseStar
      { if (F)  F->MeasurementCount()++; fittedStar = F;
      }

+  void dump(std::ostream & stream = std::cout) const
+    {
+        BaseStar::dump(stream);


I'll refrain from commenting on the uglyness of the C++ (i.e. why does dump exist in the first place).

Actually, since I've already cleaned up the interface some as part of this work, please feel free to suggest some improvements like that. What would you prefer instead of "dump"?

pschella · 2017-03-15T17:07:24Z

python/lsst/jointcal/star.cc

+    cls.def(py::init<>());
+    cls.def(py::init<double, double, double>(), "x"_a, "y"_a, "flux"_a);
+
+    // these three are actually declared in FatPoint, but we don't need that in Python


Please add DM-9814 to comment.

pschella · 2017-03-15T17:08:24Z

python/lsst/jointcal/star.cc

+    py::class_<MeasuredStar, std::shared_ptr<MeasuredStar>, BaseStar> cls(mod, "MeasuredStar");
+
+    cls.def(py::init<const BaseStar&>(), "baseStar"_a);
+}


Usually we close anonymous here. But there is some confusion on it. See upcoming slack discussion.

Should I change anything, or just leave it pending that discussion?

After further discussion with @pschella , I'm leaving this until we've settled on a standard way to handle these.

pschella · 2017-03-15T18:18:22Z

src/SimplePolyModel.cc

 {
-  const GtransfoPoly &pix2Tp=dynamic_cast<const GtransfoPoly&>(GetTransfo(Ccd));
-  const TanRaDec2Pix *proj=dynamic_cast<const TanRaDec2Pix*>(sky2TP(Ccd));
+  const GtransfoPoly &pix2Tp=dynamic_cast<const GtransfoPoly&>(GetTransfo(ccdImage));


Be careful, may throw.

In this case, throwing is fine: if that's unable to be dereferenced, something has gone terribly wrong.

pschella · 2017-03-15T18:19:03Z

include/lsst/jointcal/DistortionModel.h

@@ -21,17 +21,17 @@ public :
  virtual const Mapping* getMapping(const CcdImage &) const = 0;

  //! Assign indices to parameters involved in mappings, starting at FirstIndex. Returns the highest assigned index.
-  virtual unsigned assignIndices(unsigned FirstIndex, std::string &WhatToFit) = 0;
+  virtual unsigned assignIndices(unsigned firstIndex, std::string &whatToFit) = 0;


const for arguments? Here and in other functions below.

I only see the one const that needs adding in this class and its subclases.

pschella · 2017-03-15T18:20:47Z

python/lsst/jointcal/associations.cc

+    cls.def("addImage", &Associations::addImage);
+    cls.def("selectFittedStars", &Associations::selectFittedStars);
+
+    cls.def("getCcdImageList", &Associations::getCcdImageList);


This is not an stl container and does not return by reference I assume.

It is now an stl::list, and does return by const&.

pschella · 2017-03-15T18:23:49Z

python/lsst/jointcal/ccdImage.cc

+
+    cls.def("sizeValidForFit", &CcdImageList::sizeValidForFit);
+
+    // Make this behave like a std::list, which it is, but we can't wrap children of it easily.


Hmm, are we really really sure we can't just return this as a Python list?

This would be the first list-like object in the stack. And unless we absolutely cannot use STL containers or ndarray here it shouldn't exist.

CcdImageList is now just a typedef of std::list<shared_ptr<CcdImage>>.

pschella · 2017-03-15T18:25:44Z

python/lsst/jointcal/jointcal.py

@@ -197,14 +197,13 @@ def _build_ccdImage(self, dataRef, associations, jointcalControl):
                this calexp's filter
        """
        visit = dataRef.dataId["visit"]
-        src = dataRef.get("src", flags=lsst.afw.table.SOURCE_IO_NO_FOOTPRINTS, immediate=True)
+        src = dataRef.get("src", flags=int(lsst.afw.table.SOURCE_IO_NO_FOOTPRINTS), immediate=True)


Should this be wrapped as integer instead of enum?

Was waiting on DM-9764. That's fixed now, so I removed the cast.

pschella · 2017-03-15T18:27:03Z

python/lsst/jointcal/projectionHandler.cc

@@ -0,0 +1,54 @@
+/*
+ * LSST Data Management System


This is not the copyright statement from the template :)

Also see all other files.

Assuming you mean (the template package)[https://github.com/lsst/templates]: it wasn't clear to me that that was up-to-date with our current copyright standard (per RFC-45, or even earlier discussion). Given that RFC-45 is adopted, I think I'll wait on its implementation before changing all the copyright headers.

pschella · 2017-03-15T18:29:02Z

include/lsst/jointcal/ConstrainedPolyModel.h


  // The following routines are the interface to AstromFit
  //!
-  const Mapping* GetMapping(const CcdImage &) const;
+  const Mapping* getMapping(const CcdImage &) const;


Is this wrapped? If so please be careful with the return value policy. Should this copy or reference_internal? Should this perhaps be smart pointer? Etc. If not wrapped then outside of scope.

Same for all other functions that return raw pointers.

getMapping is not wrapped.

Thanks for the suggestion though: I went and checked, and none of the wrapped methods return pointers (raw or otherwise), so there are no problems with copy vs. reference.

pschella · 2017-03-15T18:34:39Z

python/lsst/jointcal/ccdImage.cc

@@ -66,13 +65,11 @@ void declareCcdImageList(py::module &mod) {

    cls.def("sizeValidForFit", &CcdImageList::sizeValidForFit);

-    // Make this behave like a std::list, which it is, but we can't wrap children of it easily.
+    // Make this behave like a std::list (which it is but we can't wrap std::list subclasses trivially)


Can't we make jointcal use a regular STL container (probably preferably vector) somehow?

Do we want to try to sort out some of DM-4043 now (what it would take to decide that question), or should we just try to do that in the next sprint?

ccdImageList can probably be turned into a List without too much trouble, but the *StarList are all built on CountedRefs, which are boost::intrusive_ptr, so I fear more complication there.

FWIW, I don't think we should remove custom containers just on principle. Sometimes having a custom container that can really add value if the container has some special functionality, and it can be a nice solution to the case where we can't convert directly to a built-in Python container in the wrapper layer for one reason or another.

Of course, any custom containers should delegate most of their work to an STL container (and do so via composition, not inheritance). I have no idea if these particular containers add value over their STL counterparts, but I do suspect Pierre had a good reason to choose std::list over std::vector.

One constraint is that MeasuredStar's contain some reference to their mother CcdImage.
For sake of efficiency, this is implemented as some sort of pointer, and so CcdImage's should not move in memory. The no-copy policy of CcdImages is enforced by making the copy constructor private (the comment there does not tell why. My fault). I am guessing this was the original constraint, but maybe at some point, I switched to a container of pointers for the CcdImageList, which voids the argument. So, it might just work as well with std::vector as a container. But unless a new scheme is devised to represent the MeasuredStar->CcdImage relation, copying CcdImage's should remain forbidden, which excludes a std::vector.

I unraveled CcdImageList into typedef std::list<std::shared_ptr<CcdImage> > CcdImageList;, with no obvious ill effects. I'm going to punt on the question of how to manage the various StarLists as out of scope for this ticket and deal with it in DM-4043 (which I plan to schedule for next cycle).

I suspect we'll stick with list instead of vector here, since than the algorithm has a cleanup step that removes items from the middle of the list. But lets table that until DM-4043.

Add sconscript and __init__ support Change jointcal.py to remove jointcalLib and use lsst.jointcal.blah directly. Moved *astromModel stuff into astrometryModels, similarly for *photomModel. NOTE: this suggests I should clarify the names of `simplePolyModel` and `constrainedPolyModel`, since they're really astrometry-specific models. NOTE: Several of the wrapped classes are only minimally wrapped: they will be opaque from python, but had to be wrapped as they are either returned by other methods or are default values of methods. NOTE: Remaining question is how much extra "sugar" to add (e.g. str/repr, attribute access, list-member access, etc.)

Rename distortionModel -> astrometryModel Rename photomModel -> photometryModel Rename photomFit -> photometryFit Rename astromFit -> astrometryFit

Move `point` definition into `star.cc`, since it's the base class (which should eventually disappear once DM-4044 is done) *Star have at least partially useful __str__, but still cannot be accessed when they're inside a *StarList (still waiting on DM-4043). I don't know what all the #ifndef SWIG's were for, but they seem potentially useful, and we'll never have SWIG defined, if we ever did in the first place.

CcdImageList is now just a typedef, and the "valid size" method got lifted into Associations, where it fits nicely.

parejkoj force-pushed the tickets/DM-9187 branch 2 times, most recently from a17e8e7 to 1e75587 Compare March 14, 2017 00:23

parejkoj added 3 commits March 13, 2017 17:24

Add pybind11 dependencies and remove swig

5ec6af6

Remove swig from sconscript

Rename c++ files and more variable cleanup

7a1109e

More method/argument name cleanup

f9d6beb

Cleanup DistortionMap API: produceSipWcs should be in base class.

parejkoj force-pushed the tickets/DM-9187 branch from 1e75587 to c942994 Compare March 14, 2017 00:24

pschella reviewed Mar 15, 2017

View reviewed changes

parejkoj force-pushed the tickets/DM-9187 branch from 4d9a201 to 2c2d186 Compare March 16, 2017 21:52

parejkoj added 6 commits March 16, 2017 19:44

Clarify Model and Fit names

8a399af

Rename distortionModel -> astrometryModel Rename photomModel -> photometryModel Rename photomFit -> photometryFit Rename astromFit -> astrometryFit

Add includes and imports for clarity per style guide

1f28a92

post review cleanups

8da60c6

Remove CcdImageList std::list subclass

04e3e5d

CcdImageList is now just a typedef, and the "valid size" method got lifted into Associations, where it fits nicely.

parejkoj force-pushed the tickets/DM-9187 branch from 7916b95 to 04e3e5d Compare March 17, 2017 02:51

parejkoj merged commit 04e3e5d into master Mar 17, 2017

ktlim deleted the tickets/DM-9187 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port jointcal to pybind11 tickets/DM-9187 #31

port jointcal to pybind11 tickets/DM-9187 #31

parejkoj commented Mar 11, 2017

pschella Mar 15, 2017

parejkoj Mar 15, 2017

pschella Mar 15, 2017

pschella Mar 15, 2017

parejkoj Mar 16, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

parejkoj Mar 16, 2017

pschella Mar 15, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

pschella Mar 15, 2017

parejkoj Mar 16, 2017

pschella Mar 15, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

pschella Mar 15, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

pschella Mar 15, 2017

parejkoj Mar 17, 2017

pschella Mar 15, 2017

parejkoj Mar 16, 2017

TallJimbo Mar 16, 2017 •

edited

PierreAstier Mar 16, 2017

parejkoj Mar 17, 2017


		cls.def("sizeValidForFit", &CcdImageList::sizeValidForFit);

		// Make this behave like a std::list, which it is, but we can't wrap children of it easily.

port jointcal to pybind11 tickets/DM-9187 #31

port jointcal to pybind11 tickets/DM-9187 #31

Conversation

parejkoj commented Mar 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo Mar 16, 2017 •

edited