DM-9598: Allow Transform to return a matrix of derivatives #200
Conversation
src/geom/Transform.cc
Outdated
| @@ -118,6 +120,25 @@ Transform<ToEndpoint, FromEndpoint> Transform<FromEndpoint, ToEndpoint>::getInve | |||
| } | |||
|
|
|||
| template <typename FromEndpoint, typename ToEndpoint> | |||
| Eigen::MatrixXd Transform<FromEndpoint, ToEndpoint>::getJacobian(FromPoint const &x) const { | |||
| try { | |||
| size_t const nIn = _fromEndpoint.getNAxes(); | |||
There was a problem hiding this comment.
Should you add an explicit cast to avoid compiler warnings, since getNAxes returns an int?
src/geom/Transform.cc
Outdated
| } | ||
| return jacobian; | ||
| } catch (std::bad_alloc const &e) { | ||
| std::throw_with_nested(pex::exceptions::MemoryError("Could not allocate Jacobian.")); |
There was a problem hiding this comment.
Is this desirable? If the program cannot allocate a matrix as small as this then the program is in deep trouble, and the fact that the first hint was here is not likely to be of very much interest.
Also is this a reasonable way to construct a pex_exception exception? We typically use a macro so line #s and such are recorded and that information will be missing, which worries me. (I realize one can construct the exception this way; my question is whether we ought to be doing so.)
There was a problem hiding this comment.
This is mainly for API consistency. We have a dedicated MemoryError exception, so presumably we should emit it in preference to std::bad_alloc. Although perhaps now that pybind11 gives us better exception wrapping than Swig did, we should push for deprecation of MemoryError as part of DM-9435.
As for constructing the exception, it looks like LSST_EXCEPT evaluates to an exception object rather than a throw statement, so I can do that.
tests/test_transform.py
Outdated
| assert coeffs.ndim == 2 | ||
| assert coeffs.shape == (nOut, nIn) | ||
| # Avoid spurious errors when comparing to a simplified array | ||
| return coeffs.squeeze() |
There was a problem hiding this comment.
I think it will be less confusing having makeJacobian return the expected 2-dimensional array. The fact that the pybind11 wrapper may return something else for the Jacobian is a misfeature (one I hope we can eventually figure out a workaround for) that we need not emulate here. Instead consider putting the squeeze where the two matrices are compared.
There was a problem hiding this comment.
Glad to hear it. The impression I got from @TallJimbo on #dm-pybind11 was that numpy changing the dimensions was to be considered a feature rather than a bug.
There was a problem hiding this comment.
numpy generally (or at least occasionally) considers that a feature; I'm not sure I do, but I didn't want to try to fight numpy's conventions.
There was a problem hiding this comment.
@TallJimbo I filed https://jira.lsstcorp.org/browse/DM-10011 in hopes you might change your mind about fighting this. It is very undesirable behavior in this case and it's a pain to have to add a python wrapper to work around it (especially since it is a templated class with many instantiations).
tests/test_transform.py
Outdated
| @@ -609,12 +636,45 @@ def checkInverseFrameSet(self, clsTransform, frameIn, frameOut): | |||
| self.assertEqual(reverseFrames.getFrame(4).getIdent(), "currFrame", | |||
| msg=baseMsg) | |||
|
|
|||
| def checkGetJacobian(self, fromName, toName): | |||
| """Test Transform<fromName>To<toName>.getInverse | |||
kfindeisen
force-pushed
the
tickets/DM-9598
branch
2 times, most recently
from
588dc43
to
d0630d5
Compare
kfindeisen
force-pushed
the
tickets/DM-9598
branch
from
d0630d5
to
303d572
Compare
This commit implements
Transform::getJacobian. The unit tests intest_transform.pywere modified slightly to make the test transform non-linear, so that the Jacobian is not constant. A supplementary test was added astest_transform.ccto make absolutely sure that the matrix dimensions are what they should be, since conversion to ndarray loses some of that information.