DM-8439: Add wrapper on astshim to take point lists #182

r-owen · 2017-02-28T19:34:18Z

No description provided.

kfindeisen · 2017-03-01T23:35:30Z

include/lsst/afw/geom/SpherePoint.h

+     *
+     * @exceptsafe Provides strong exception guarantee.
+     */
+    SpherePoint();


I'd still prefer not to let SpherePoint be default-constructible if it can be avoided; the stated default, while better than any of the alternatives, is not as obvious as it could be because the default point is (0, 0) in lon-lat representation but (1, 0, 0) in vector representation.

Do you use default constructibility anywhere? I tried looking for explicit variable declarations but didn't find any.

If you do keep the default constructor, consider declaring it noexcept.

parejkoj

The overall class designs seem reasonable to me. I would suggest having another pybind11 master take a look, since I don't really know what to look for in that code.

Some clarification questions, several things about the uses of asserts.

Also, you may want to add self.longMessage=True to the python test setUp(), so that the msg= kwargs don't overwrite the default messages. See here: https://docs.python.org/2/library/unittest.html#unittest.TestCase.longMessage

parejkoj · 2017-03-01T22:22:10Z

include/lsst/afw/geom/Endpoint.h

+- Point3Endpoint is used for Point3D data
+- SpherePointEndpoint for SpherePoint data
+- GenericEndpoint is used when no other form will do; its LSST data type
+  is identical to the type used for ast::Mapping.tran.


"is identical" -> "must be identical"?

I prefer my wording. I consider it an explanation of the design and a suggestion of a place to look for more information.

parejkoj · 2017-03-01T22:28:52Z

include/lsst/afw/geom/Endpoint.h

+template <typename PointT, typename ArrayT>
+class BaseEndpoint {
+public:
+    using Point = PointT;


Out of curiosity, why do you use these "using"s here, instead of either just using the T names, or calling the types the name without the T in the first place?

This makes PointT available to users as BaseEndpoint::Point. I suspect the template type name and the class attribute can have the same name, but I worry that using Point = Point looks too strange. @kfindeisen may have an opinion about this.

Ah, ok. That makes sense as a technical reason why to do it this way.

I don't actually know whether using Point = Point would compile (though it probably would). @parejkoj, the LSST style guide recommends the PointT name (and disambiguating that we don't mean afw::geom::Point is certainly relevant here).

parejkoj · 2017-03-01T22:33:27Z

include/lsst/afw/geom/Endpoint.h

+
+    Do not obother to check the number of axes because that is done elsewhere.
+
+    The base implementation does nothing.


Is this line redundant, since it's virtual?

This is a default behavior that derived classes inherit and can choose to override. As such it might do something useful, but in this case it does nothing.

parejkoj · 2017-03-01T22:36:18Z

include/lsst/afw/geom/Endpoint.h

+        return data.size();
+    }
+
+    int _getNPoints(ndarray::Array<double, 2, 2> const & data) const {


Do you also need a _getNPoints for <double, 1, 1>?

No because <double, 1, 1> is used to represent a single point. (Also these private methods are simply used as convenience methods for other code; the fact that the code compiles and passes unit tests is a fairly strong indication that it is not missing such methods).

parejkoj · 2017-03-01T22:41:13Z

include/lsst/afw/geom/Endpoint.h

+    */
+    explicit BaseEndpoint(int nAxes) : _nAxes(nAxes) {}
+
+    virtual ~BaseEndpoint() {};


Do you need to explicitly implement the default/delete copy and move constructors here, per RFC-209?

parejkoj · 2017-03-02T01:07:57Z

ups/afw.table

@@ -15,6 +15,7 @@ setupRequired(astropy)
 setupRequired(python_future)
 setupRequired(log)
 setupRequired(pybind11)
+setupRequired(astshim)


And they never thought there'd be a day when afw depended on AST!

parejkoj · 2017-03-02T01:08:28Z

tests/test_transform.py

+            # check invalid numbers of output for the given toName
+            for badNout in getBadNAxes(toName):
+                badPolyMap = makePolyMap(nIn, badNout)
+                with self.assertRaises(Exception):


Which Exception?

parejkoj · 2017-03-02T01:09:58Z

tests/test_transform.py

+                inPoint = fromEndpoint.pointFromData(rawInPoint)
+                outPoint = transform.tranForward(inPoint)
+                rawOutPoint = toEndpoint.dataFromPoint(outPoint)
+                self.assertTrue(np.allclose(rawOutPoint, polyMap.tranForward(rawInPoint)))


self.assertFloatsAlmostEqual and msg= here, and the other asserts below.

parejkoj · 2017-03-02T01:10:24Z

tests/test_transform.py

+                self.assertTrue(np.allclose(rawInversePoint, frameSet.tranInverse(rawOutPoint)))
+
+                # forward transformation of an array of points
+                nPoints = 7  # arbitrary


Thanks for the comment.

parejkoj · 2017-03-02T01:10:37Z

tests/test_transform.py

+        for badNin in getBadNAxes(fromName):
+            for nIn in getNaxes(toName) + getBadNAxes(toName):
+                badPolyMap = makePolyMap(badNin, nOut)
+                with self.assertRaises(Exception):


Which Exception?

parejkoj

Thanks for adding the output operator. That'll be helpful when debugging in python.

parejkoj · 2017-03-03T23:27:20Z

I think you still need to convert all the with self.assertRaises(Exception) into catching only the specific exception that should be raised in those cases. Otherwise, strange errors that shouldn't occur would be masked by the test.

parejkoj · 2017-03-03T23:30:05Z

And I guess the remaining assertTrue(np.allclose()) are due to the assertFloatsAlmostEqual bug re: lists that you filed earlier? In that case, I'd suggest using np.testing.assert_almost_equal(), with a note about why that includes the ticket number (and mention it in the ticket you filed).

kfindeisen

I have a few requests for more documentation and a few suggestions for code cleanup, but I don't see any major issues in the pybind11 wrappers.

kfindeisen · 2017-03-09T19:24:38Z

python/lsst/afw/geom/endpoint.cc

+    cls.def("__str__", [](Class const& self) {
+        std::ostringstream os;
+        os << self;
+        return os.str();


Personally I prefer buffer as more informative than os, but we can worry about when we standardize post-RFC-298.

I'd be surprised and disappointed if we felt we had to dictate the variable name in our standards.

Not that kind of standard. Pim expressed an interest in reducing the code duplication in our current __str__ and __repr__ implementations, presumably by creating a single centralized function to replace these lambdas.

Got it. I think that will work out to not wrapping __repr__ at all (which I am violating to provide a slightly nicer class name) and making __str__ = operator<< (which it is, so it can trivially modified to call the standard function when we have one).

kfindeisen · 2017-03-09T19:27:31Z

python/lsst/afw/geom/endpoint.cc

+    using Class = typename PyClass::type;  // C++ class associated with pybind11 wrapper class
+    cls.def("__repr__", [](Class const& self) {
+        std::ostringstream os;
+        os << "lsst.afw.geom." << self;


This is only correct if the output of operator<< begins with the name of the class; perhaps document that as a requirement of <<?

kfindeisen · 2017-03-09T19:33:37Z

python/lsst/afw/geom/endpoint.cc

+    cls.def("makeFrame", [](Class const & self) {
+        auto frame = self.makeFrame();
+        return frame->copy();
+    });


Comment on why you don't wrap getNPoints, *From*?

I assume we don't ever plan to create python-only subclasses, so we don't need the protected methods?

I originally didn't wrap getNPoints and etc. because they were not implemented in this base class. But I think it's considered better to wrap them, so I will.

I don't anticipate us making Python-only subclasses, and the protected methods aren't essential to doing so.

kfindeisen · 2017-03-09T20:19:31Z

python/lsst/afw/geom/endpoint.cc

+    py::class_<Class, std::shared_ptr<Class>, BaseEndpoint<Point, Array>> cls(
+            mod, ("_BaseVectorEndpoint" + suffix).c_str());
+
+    cls.def(py::init<int>(), "nAxes"_a);


Isn't BaseVectorEndpoint a base class? Why do you need the constructor?

I'll remove it. Also, I'll make the constructor for BaseEndpoint and BaseVectorEndpoint protected, since neither is intended to be used directly.

kfindeisen · 2017-03-09T20:29:34Z

python/lsst/afw/geom/endpoint.cc

+    using Point = typename Class::Point;
+    std::stringstream os;
+    os << "Point" << N;
+    std::string pointNumStr = os.str();


An iostream is overkill for this. Use std::to_string instead.

kfindeisen · 2017-03-09T20:37:30Z

python/lsst/afw/geom/endpoint.cc

+    cls.def("makeFrame", [](Class const & self) {
+        auto frame = self.makeFrame();
+        return frame->copy();
+    });


Code duplication with declareBaseEndpoint; factor into a common function.

kfindeisen · 2017-03-09T20:47:35Z

python/lsst/afw/geom/transform.cc

+    os << pyClassName;
+    auto const frameSet = self.getFrameSet();
+    os << "[" << frameSet->getNin() << "->" << frameSet->getNout() << "]";
+    return os.str();


You might want to add a comment noting that all the distinguishing information is in pyClassName; at first I thought the result would be something like "Transform[2->2]", which is not particularly helpful.

kfindeisen · 2017-03-09T21:18:50Z

python/lsst/afw/geom/transform.cc

+    cls.def("tranForward", (ToPoint (Class::*)(FromPoint const &) const) &Class::tranForward, "point"_a);
+    cls.def("tranInverse", (FromArray (Class::*)(ToArray const &) const) &Class::tranInverse, "array"_a);
+    cls.def("tranInverse", (FromPoint (Class::*)(ToPoint const &) const) &Class::tranInverse, "point"_a);
+    /// repr format is lsst.afw.geom.<ClassName>[<nIn>-><nOut>]


Doxygen comments should not be used inside function bodies. Did you mean //? [Same for __str__]

Add SpherePoint(double const lonLatRad[2]) for efficient construction from raw data.

In Python these classes are named Transform_fromPrefix_To_toPrefix_ where _fromPrefix_ and _toPrefix_ are endpoint prefixes such as "Generic", "Point3" or "SpherePoint".

pschella force-pushed the tickets/DM-8467 branch from 846abd0 to 117fb96 Compare March 1, 2017 17:09

r-owen force-pushed the tickets/DM-8439 branch from d7daa91 to 1d6e174 Compare March 1, 2017 21:52

kfindeisen reviewed Mar 1, 2017

View reviewed changes

parejkoj requested changes Mar 2, 2017

View reviewed changes

TallJimbo force-pushed the tickets/DM-8467 branch from 36a5be1 to 24a8200 Compare March 3, 2017 17:06

r-owen force-pushed the tickets/DM-8439 branch 3 times, most recently from aa0f4e1 to 5d8f7d7 Compare March 3, 2017 21:42

parejkoj reviewed Mar 3, 2017

View reviewed changes

pschella force-pushed the tickets/DM-8467 branch from 2cd8185 to 5f4149e Compare March 7, 2017 02:45

TallJimbo force-pushed the tickets/DM-8467 branch 2 times, most recently from e61236b to a335cbb Compare March 7, 2017 18:12

r-owen force-pushed the tickets/DM-8439 branch 2 times, most recently from 53f479c to 0967862 Compare March 8, 2017 00:15

r-owen changed the base branch from tickets/DM-8467 to master March 8, 2017 00:41

parejkoj approved these changes Mar 8, 2017

View reviewed changes

kfindeisen approved these changes Mar 9, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-8439 branch 5 times, most recently from 7d60ddd to c83d116 Compare March 11, 2017 15:32

r-owen added 4 commits March 11, 2017 07:35

Add new SpherePoint constructor

fe68861

Add SpherePoint(double const lonLatRad[2]) for efficient construction from raw data.

Add astshim to dependencies

cc9bdaf

Add Endpoint

5992c82

Add Transform<FromEndpoint, ToEndpoint>

b1ca1a8

In Python these classes are named Transform_fromPrefix_To_toPrefix_ where _fromPrefix_ and _toPrefix_ are endpoint prefixes such as "Generic", "Point3" or "SpherePoint".

r-owen force-pushed the tickets/DM-8439 branch from c83d116 to b1ca1a8 Compare March 11, 2017 15:35

r-owen merged commit b1ca1a8 into master Mar 13, 2017

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


		Do not obother to check the number of axes because that is done elsewhere.

		The base implementation does nothing.

DM-8439: Add wrapper on astshim to take point lists #182

DM-8439: Add wrapper on astshim to take point lists #182

Conversation

r-owen commented Feb 28, 2017

Choose a reason for hiding this comment

parejkoj left a 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

parejkoj left a comment

Choose a reason for hiding this comment

parejkoj commented Mar 3, 2017

parejkoj commented Mar 3, 2017

kfindeisen left a 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

r-owen Mar 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Mar 10, 2017 • edited

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

r-owen Mar 10, 2017 •

edited

r-owen Mar 10, 2017 •

edited