meas_astrom/DM-3549: new SIP fitter #47

TallJimbo · 2016-10-03T17:27:19Z

This adds a new fitter for SIP distortions in the form of a number of C++ classes (several polynomial-based transform classes and a fitter) and a new Python task that is intended as a drop-in replacement for FitTanSipWcsTask.

Some caveats:

I have not yet enabled the new task, FitSipDistortionTask, as the default; I think it needs a bit more testing before we do that. I do however expect it to fully replace FitTanSipWcsTask eventually, so I have not tried to more strictly define the interface for WCS fitters or add a registry as we do with other pluggable tasks. I also have ignored some (minor) code duplication between the tasks (especially their config classes) that would be better to clean up with common base class if we expected them to coexist for a long time.
The new transform classes do not inherit from XYTransform or any other common base class, and that probably limits their usefulness to this particular application. I think we'll want to refactor that when we have our new WCS/transform library, but doing it before then is probably premature.

r-owen

I have gotten through the .h and .i files. I'll review more tomorrow.

So far my main request is to document some of the private code.

r-owen · 2016-10-05T21:12:50Z

include/lsst/meas/astrom/detail/polynomialUtils.h

+
+namespace lsst { namespace meas { namespace astrom { namespace detail {
+
+inline int computePackedOffset(int order) {


Please document this and all other functions and classes in this file. This one is not so obscure, but I have no idea what some of the things in this file do.

Done (locally; will commit and push after aggregating minor comments).

r-owen · 2016-10-05T21:13:03Z

include/lsst/meas/astrom/detail/polynomialUtils.h

+namespace lsst { namespace meas { namespace astrom { namespace detail {
+
+inline int computePackedOffset(int order) {
+    return (order*(order+1))/2;


Space around +?

r-owen · 2016-10-05T21:22:23Z

include/lsst/meas/astrom/ScaledPolynomialTransformFitter.h

+     *  @param[in] bbox             Bounding box for the grid in the input
+     *                              coordinates of the toInvert transform
+     *                              (or equivalently the output coordinates
+     *                              of the transform to be fit).


Can you provide a concrete example of what the bbox will be for the typical case? Will it be the bbox of the exposure on which the sources were measured? The smallest bbox that encloses all sources after removing distortion? Or? Put another way: can you suggest how the user would usually compute this bbox?

In the way it's used in FitSipDistortionTask, the bounding box has to be in intermediate world coordinates, and given that we only start with the bounding box of the image and we're trying to find the mapping from pixel coordinates to intermediate world coordinates here, our only choice is to transform the image bounding box using (part of) the initial guess WCS.

I don't think it's appropriate to include that description in the docs for fromGrid, since it's more general than that, but I will add code comments to FitSipDistortionTask where I'm computing the bbox.

r-owen · 2016-10-05T21:27:34Z

include/lsst/meas/astrom/ScaledPolynomialTransformFitter.h

+
+    /**
+     *  Perform a linear least-squares fit of the polynomial coefficients.
+     *


Could you say a few words (perhaps in the class description) how you might call this to perform a fit, e.g. when to call fit, updateIntrinsicScatter and rejectOutliers, in what order, how often, etc.

Done. I've added a bit to each of these functions and a code block to the class docs describing the typical iteration pattern.

r-owen · 2016-10-05T21:28:35Z

include/lsst/meas/astrom/ScaledPolynomialTransformFitter.h

+    /**
+     *  Return the current estimate of the intrinsic scatter.
+     */
+    double getIntrinsicScatter() const { return _intrinsicScatter; }


Could you say a few words (preferably in the doc for this function or the previous) why you have separate updateIntrinsicScatter and getIntrinsicScatter methods? Naively I would expect you could manage with just one of them. Forgetting to call the former before calling the latter seems like an accident waiting to happen.

The intrinsic scatter is actually included in the uncertainty on the data points, so one might want to call this before updateIntrinsicScatter to obtain the scatter used in the last call to fit or rejectOutliers. Calling it after updateIntrinsicScatter tells you the scatter that will be used in the next call to fit or rejectOutliers. (I've added that to the docs for getIntrinsicScatter).

r-owen · 2016-10-06T00:08:13Z

include/lsst/meas/astrom/SipTransform.h

+     *  Convert a PolynomialTransform to an equivalent SipReverseTransform.
+     *
+     *  @param[in]   poly           PolynomialTransform to convert.
+     *  @param[in]   pixelOrigin    CRPIX @f$(u_0,v_0)@f$


Again, anywhere you mention CRPIX please be explicit about the zero convention (that's safer than putting it in one place and hoping folks stumble across it).

r-owen · 2016-10-06T00:10:14Z

include/lsst/meas/astrom/SipTransform.h

+};
+
+/**
+ *  Create a new TanWcs from a pair of SIP transforms and the sky origin.


I suggest you say "TAN SIP WCS" instead of a "TanWcs"; the latter is a poorly named class that should go away; the former is a more general concept that should remain after the WC rewrite.

r-owen · 2016-10-06T00:12:04Z

python/lsst/meas/astrom/astromLib.i


 %include "lsst/meas/astrom/matchOptimisticB.h"

+%include "std_pair.i"
+
+namespace std { // have to do this in namespace block so Swig can find size_t


weird. You can't just use std::size_t?

Correct. That was what I tried first. I think I could have also fixed it by not using std:: in the C++ header file, but I'm not sure that's valid C++ even though my compiler allows it.

r-owen · 2016-10-06T00:13:01Z

python/lsst/meas/astrom/astromLib.i


 %include "lsst/meas/astrom/matchOptimisticB.h"

+%include "std_pair.i"


If this could safely go higher (e.g. right near %include "ndarray.i" I think it would make the code a bit easier to read; we usually put standard and 3rd party includes together near the top when we can get away with it.

Another option worth considering is moving the new code into a separate .i file. There is a fair amount of it, so it may enhance readability.

I'll refactor into a new file.

r-owen · 2016-10-06T00:13:46Z

python/lsst/meas/astrom/astromLib.i

+%template(DoubleSizePair) pair<double,size_t>;
+}
+
+%declareNumPyConverters(ndarray::Array<double const,2,2>)


Here's another line I suggest that you move way up with the standard imports (and typically vector templates and such) if that's safe to do.

I was actually planning to put this in the custom .i file for the transform classes, since it's only needed by them.

r-owen

Python code reviewed. One debug "or True" needs to be removed, but otherwise just minor stuff, including requests for clarification.

r-owen · 2016-10-06T17:54:12Z

python/lsst/meas/astrom/fitSipDistortion.py

+        default=1
+    )
+    maxScatterArcsec = lsst.pex.config.RangeField(
+        doc="maximum median scatter of a WCS fit beyond which the fit fails (arcsec); " +


(picky) please caplitalize "Maximum" for consistency with the other doc strings.

Done (also capitalized "order" and "number" for the same reason).

r-owen · 2016-10-06T17:55:08Z

python/lsst/meas/astrom/fitSipDistortion.py

+    )
+    refUncertainty = lsst.pex.config.Field(
+        doc="RMS uncertainty in reference catalog positions, in pixels.  Will be added " +
+        "in quadrature with measured uncertainties in the fit.",


Please consider indenting continuation strings, for improved readability.

r-owen · 2016-10-06T17:56:25Z

python/lsst/meas/astrom/fitSipDistortion.py

+        default=100,
+    )
+    gridBorder = lsst.pex.config.Field(
+        doc=("When setting the gird region, how much to extend the image "


Please remove the parens as they are unnecessary and confusing (hinting that doc is being set to a tuple). I suggest using the same format for all multi-line strings, with a slight preference for the format you used above with "+" at the end of each line, since the "+", though optional, makes the intent clearer.

r-owen · 2016-10-06T17:58:20Z

python/lsst/meas/astrom/fitSipDistortion.py

+
+    FitSipDistortionTask is a drop-in replacement for
+    :py:class:`lsst.meas.astrom.FitTanSinWcsTask`.  It is built on fundamentally
+    stronger fitting algorithms, but has received significantly less testing.


If you have not already done so, please file a ticket to remove this caveat and make this the default fitter once it has been adequately tested. Otherwise this comment may live long past its relevant date.

r-owen · 2016-10-06T18:01:46Z

python/lsst/meas/astrom/fitSipDistortion.py

+
+     - We set the CRVAL and CRPIX reference points to the mean position, while
+       holding the CD matrix fixed to its input value.  This work is done by
+       the makeInitialWcs method.


Summarizing what this means in English would be helpful. I think you mean that you adjust the mean position of the WCS. That said, I'm surprised that you choose to adjust both CRVAL and CRPIX instead of just one of them. I would have thought one would be treated as a given.

I thought this was a summary, though I see that I just said "mean position" when I should have said "mean position of all the matches". Does that clarify things?

I think we could add a config option to leave CRVAL and/or CRPIX fixed at the positions from the given initial WCS, but this matches the behavior of FitTanSipWcsTask and I didn't see anything wrong with that.

If it matches current behavior then I agree there seems no strong need to allow keeping them fixed.

r-owen · 2016-10-06T18:39:05Z

python/lsst/meas/astrom/fitSipDistortion.py

+        # Fit the forward mapping to a grid of points created from the reverse
+        # transform. Because that grid needs to be defined in intermediate
+        # world coordinates, we first grow the bounding box and then apply the
+        # initial WCS's CRPIX and CD matrix.


I don't understand this explanation, starting from "Because that grid...". What is the connection between using intermediate world coordinates and needing to grow the bbox? Or is it that you intended to grow the bbox all along, but the sequencing of growing first, then applying... is driven by using intermediate world coordinates?

Also, I suggest replacing "needs to be" with "is" (or similar) because it is more of a choice (albeit an important choice driven by experiment) rather than a fundamental requirement.

I'm growing because I'm worried the transform I use to generate the box is only approximate, and I'd rather get a too-big box than a too-small one. I'll clarify.

r-owen · 2016-10-06T18:42:06Z

python/lsst/meas/astrom/fitSipDistortion.py

+        # initial WCS's CRPIX and CD matrix.
+        gridBBoxPix = lsst.afw.geom.Box2D(bbox)
+        gridBBoxPix.grow(self.config.gridBorder)
+        gridBBox = lsst.afw.geom.Box2D()


Please consider a longer name to better differentiate gridBBox from bbox, since they are in different coordinate systems; perhaps gridBBoxIwc.

r-owen · 2016-10-06T19:02:37Z

python/lsst/meas/astrom/fitSipDistortion.py

+            If provided then coords are updated with the new WCS;
+            otherwise only the coords for sources in matches are updated.
+            Required fields are "slot_Centroid_x", "slot_Centroid_y",
+            "coord_ra", and "coord_dec".


Don't you also need centroid uncertainty?

Good catch; fixed.

r-owen · 2016-10-06T19:14:42Z

python/lsst/meas/astrom/fitSipDistortion.py

+                sErr = record.get(srcErrKey)
+                sEllipse = lsst.afw.geom.ellipses.Quadrupole(sErr[0, 0], sErr[1, 1], sErr[0, 1])
+                disp.dot(sEllipse, sx, sy, ctype=colors[1])
+        print("Dropping into debugger to allow inspection of display. Type 'continue' when done.")


Why not just display each iteration in a new frame without stopping? It seems less disruptive.

If you think that's the more common pattern, I'm happy to change it. This is the first time I've actually written interactive display code in a pipeline task.

It's a slightly delicate issue. Dropping into the debugger is fairly rare, but by no means unheard of. However, the current display code handles frame numbers rather poorly because it does not support any kind of auto-increment. It's easy enough to keep track of a local frame number, but if later code wants to display something in a different frame...

Which reminds me: display is often an integer that states the starting frame number (though if we had auto-increment we'd rarely want that).

I've added separate lsstDebug variables for the frame and whether to pause (I've also seen that as a way to specify the (starting) frame).

r-owen · 2016-10-06T19:17:07Z

python/lsst/meas/astrom/fitSipDistortion.py

+        matches with no estimated Wcs, and the input Wcs is a wild guess.
+        We're using the best of each: positions from the matches, and scale
+        from the input Wcs.
+


This really surprises me. The initial WCS ought to include an estimate of optical distortion, and so ought to be pretty decent. Still...I assume you found you needed to do this instead of trusting the initial WCS. I just wish I knew why.

I actually just stole this method (including that doc text) from the old WCS fitter task. If it's no longer accurate, I'll change it.

I've softened the docs here to make it clear that we're being defensive rather than explicitly distrusting the input, and that it's the position we're least certain about.

pschella · 2016-10-06T21:45:18Z

include/lsst/meas/astrom/SipTransform.h

+        std::swap(_pixelOrigin, other._pixelOrigin);
+        std::swap(_cdMatrix, other._cdMatrix);
+        _poly.swap(other._poly);
+    }


Any particular reason you are not using ADL for swap?

I have to admit I'm not entirely clear when it's advantageous to do so; just I figured I didn't need it here. Are you advocating it just because you think it's a good pattern to follow, or is there a benefit I might not be aware of?

pschella · 2016-10-06T21:46:04Z

include/lsst/meas/astrom/PolynomialTransform.h

+    ndarray::EigenView<double,2,2> _xCoeffs;
+    ndarray::EigenView<double,2,2> _yCoeffs;
+    mutable Eigen::VectorXd _u; // workspace for operator() and linearize
+    mutable Eigen::VectorXd _v;


Thread safe?

Hmm., I think I'll just document that instance of this class should be confined to a single thread, as I think that's probably how it ought to be used. Ultimately I expect it to be a moot point when we switch to using AST for these classes.

Edited: I was more careful than I remembered being: BinomialMatrix is already threadsafe.

r-owen

Finished the .cc files. Still have the unit tests.

r-owen · 2016-10-06T21:18:24Z

src/PolynomialTransform.cc

+PolynomialTransform PolynomialTransform::convert(SipForwardTransform const & other) {
+    PolynomialTransform poly = other.getPoly();
+    poly._xCoeffs(1, 0) += 1;
+    poly._yCoeffs(0, 1) += 1;


Would you consider a brief comment as to why you are incrementing these coefficients?

Done. It's to account for the extra terms outside the sum in the SIP definition formula; you can fold those into the sum by adding one to these two coefficients.

r-owen · 2016-10-06T21:23:24Z

src/PolynomialTransform.cc

+        throw LSST_EXCEPT(
+            pex::exceptions::LengthError,
+            "PolynomialTransform order must be >= 0"
+        );


If order < 1 then won't the ndarray::allocate(...) statements in the assignment list have caused problems by the time you get here? If so, then perhaps you should allocate here rather than in the assignment list.

Good catch. Done (need to move the Eigen initializations too, I think).

r-owen · 2016-10-06T21:24:57Z

src/PolynomialTransform.cc

+    if (xCoeffs.getShape() != yCoeffs.getShape()) {
+        throw LSST_EXCEPT(
+            pex::exceptions::LengthError,
+            "X and Y coefficient matrices must have the same shape."


Please print the shapes

r-owen · 2016-10-06T21:25:31Z

src/PolynomialTransform.cc

+    if (_xCoeffs.cols() != _xCoeffs.rows()) {
+        throw LSST_EXCEPT(
+            pex::exceptions::LengthError,
+            "Coefficient matrices must be triangular, not trapezoidal."


trapezoidal? Aren't they rectangular? In any case, please print the offending values.

For matrices, trapezoidal is to triangular as rectangular is to square. They're triangular because we don't want nonzero terms with p+q > order, though it's not worth out time to test that.

Offending values printed.

r-owen · 2016-10-06T21:32:55Z

src/PolynomialTransform.cc

+    ScaledPolynomialTransform result(
+        sipForward.getPoly(),
+        afw::geom::AffineTransform(afw::geom::Point2D() - sipForward.getPixelOrigin()),
+        afw::geom::AffineTransform(sipForward.getCDMatrix())


I suggest Point2D(0, 0) (or 0.0, 0.0) so the reader doesn't have to look up what the default Point2D constructor does.

Fine; done.

r-owen · 2016-10-06T22:21:22Z

src/ScaledPolynomialTransformFitter.cc

+#include "lsst/afw/table/Match.h"
+#include "lsst/afw/math/LeastSquares.h"
+
+#define LSST_ScaledPolynomialTransformFitter_TEST_IN_PLACE 1


Please document what this does. Do you really want it left true for production code?

Good catch. I do not want it left true for production code. I've added an extensive comment.

r-owen · 2016-10-06T22:22:26Z

src/ScaledPolynomialTransformFitter.cc

+    Eigen::ArrayXd fyy = 1.0/(syy - sxy.square()/sxx);
+    Eigen::ArrayXd fxy = -(sxy/sxx)*fyy;
+#ifdef LSST_ScaledPolynomialTransformFitter_TEST_IN_PLACE
+    assert((sxx*fxx + sxy*fxy).isApproxToConstant(1.0));


I suspect we will disagree about this, but this section is presently on by default and I'm very nervous about asserts being run in production code. I'd be much happier with raising an exception so the user gets a more useful message and other code has a chance to continue if the fitter fails.

Hopefully this is resolved by the above change to disable this block entirely.

r-owen · 2016-10-06T22:27:36Z

src/ScaledPolynomialTransformFitter.cc

+    // Function that computes the -log likelihood of the current deltas with
+    // the variance modeled as described above.
+    auto logLikelihood = [this](double intrinsicVariance) {
+        // Uncertainties in the table right now include the old intrinsic scatter; need to


Is it important to use this syntax instead of defining a named function (perhaps in the anonymous namespace)? I think the latter would be easier to read.

A lambda defined here has access to ScaledPolynomialTransformFitter's private members, which a free function in an anonymous namespace would not. A private member function would have access to those private members, but then wouldn't be usable as a functor as I could pass to brent_find_minima. So I'd have to have a lambda function anyway to call the private member function, or do a lot of friending.

I see. And C++ (even C++11) doesn't support defining a function inside a function.

I think it does support that, actually, at least in some limited sense, but it's regarded as strongly deprecated in favor of lambdas.

r-owen · 2016-10-06T22:29:57Z

src/ScaledPolynomialTransformFitter.cc

+    OutlierRejectionControl const & ctrl
+) {
+    if (!_keys.valid.isValid()) {
+        throw LSST_EXCEPT(


I already admitted to not knowing what _keys.valid is (or _keys) but this test really surprises me. The very fact that the _keys.valid.isValid() is true tells you that you are trying to reject outliers? Why? A comment here might be helpful.

_keys.valid is a Key for the field that's true if an object is outlier-rejected and false if it isn't. But if we construct with fromGrid, there's no uncertainty and hence no outlier rejection, and that field never gets added to the schema.

(I've added that as a comment).

r-owen · 2016-10-06T22:46:06Z

src/SipTransform.cc

+    if (!sipForward.getPixelOrigin().asEigen().isApprox(sipReverse.getPixelOrigin().asEigen())) {
+        throw LSST_EXCEPT(
+            pex::exceptions::InvalidParameterError,
+            "SIP forward and reverse transforms have inconsistent CRPIX"


Print the values?

r-owen

Done. This is a really nice set of tests.

The main thing I ask is to make sure your python code is compatible with both python 2 and python 3 and that it flake8 reports it as clean. I believe meas_astrom has been updated to py3 compatibility, so you can test your new code with Jenkins py3 by building product lsst_py3 and skipping the demo.

r-owen · 2016-10-07T17:12:21Z

tests/testTransforms.py

+#
+
+import unittest
+import numpy


Please change to import numpy as np

r-owen · 2016-10-07T17:14:52Z

tests/testTransforms.py

+class TransformTestMixin(object):
+
+    def checkTransformEquivalence(self, a, b, rtol=1E-8):
+        for i in xrange(10):


Please use range and from builtins import range for python 3 compatibility. More generally, I suggest a futurize stage 2 pass on this code to make sure it all should be python 3 compatible.

Fixed. I'll run futurize if I can get it working quickly (haven't tried it before), and I'll certainly run the py3 Jenkins before merging to make sure I haven't missed anything.

r-owen · 2016-10-07T17:18:50Z

tests/testTransforms.py

+
+
+class TransformTestMixin(object):
+


To improve the readability I suggest adding a makeRandom method that raises NotImlpementedError or using abc to make this an abstract base class, and documenting makeRandom (the API and the need to override it).

r-owen · 2016-10-07T17:21:03Z

tests/testTransforms.py

+        self.assertClose(p.getXCoeffs(), xc, atol=0, rtol=0)
+        self.assertClose(p.getYCoeffs(), yc, atol=0, rtol=0)
+        # Test that the coefficients are not a view.
+        xc[0, 0] = 50.0


It is not self-evident that the existing value cannot be 50. Please consider incrementing the existing value (after saving the initial value to use in the comparison test).

r-owen · 2016-10-07T17:26:11Z

tests/testTransforms.py

+        composed1 = lsst.meas.astrom.compose(poly, affine)
+        composed2 = lsst.meas.astrom.compose(affine, poly)
+        self.checkTransformEquivalence(composed1, lambda p: poly(affine(p)))
+        self.checkTransformEquivalence(composed2, lambda p: affine(poly(p)))


The test doesn't quite seem to do what is advertised: check that the result of composing gives the expected result. I suggest you define a python transform that applies affine and then poly (or vice-versa) and check equivalence (similar to what ScaledPolynomialTransformTestCase .testConstruction does below)

I think you must have misread this - it's already doing exactly what you've asked; the "python transform that applies affine and then poly" is what the first of these lambdas is.

r-owen · 2016-10-07T17:32:34Z

tests/testTransforms.py

+        def t2(p):
+            sky = lsst.afw.coord.IcrsCoord(crval + lsst.afw.geom.Extent2D(p), lsst.afw.geom.degrees)
+            return wcs.skyToPixel(sky)
+        self.checkTransformEquivalence(t1, t2)


Is it worth also checking fwd in the same way, with another pair of functions?

That's actually a lot harder - it requires actually inverting the polynomial part of rev (note that fwd isn't actually the inverse of rev right now). I do essentially that down in testFromGrid.

r-owen · 2016-10-07T17:35:20Z

tests/testTransforms.py

+    poly = makeRandomPolynomialTransform(order, sip=False)
+    return SipReverseTransform(origin, cd, poly)
+
+


This is a very nice set of tests. However, I feel as if too much is contained in one file. I suggest you move the common code to a library module, such as lsst.meas.astrom.testUtils, including TransformTestMixin. Then divide the remaining subclasses of TransformTestMixin into individual test files with more descriptive filenames.

Also, this test has a number of minor flake8 linter errors. Please run flake8 on this and all the other new python code you added and touched.

While it is a long file, I'd prefer to keep these together - I don't think there's anything intrinsically bad about having a few hundred lines of code in one file, and the test utility code is just limited to these tests rather than other stuff in meas_astrom. And, as we've discussed, I expect all of these classes to eventually be replaced by new AST-like things, and if these tests remain valuable then, I expect they'll be moved between packages and it will all be easier to deal with if it's all in one file.

I will run flake8.

r-owen · 2016-10-07T17:42:24Z

tests/testTransforms.py

+
+class TransformTestMixin(object):
+
+    def checkTransformEquivalence(self, a, b, rtol=1E-8):


Please rename to assert..., perhaps assertTransformsNearlyEqual.

Please also consider moving this to afw.geom.testUtils, such that it becomes a standard part of lsst.utils.tests.TestCase with a name that reflects the fact that the transforms it tests are 2d (e.g. assert2DTransformsNearlyEqual) and with a user-specified domain.

I've renamed it as you've suggested.

I don't think I'm going to move it somewhere more general right now, though, because I think the domain/range questions are very important for testing transforms more generally, and I don't want to put in the work right now to make this utility function handle more general domains and ranges.

r-owen · 2016-10-07T17:46:31Z

tests/testTransforms.py

+
+    def testFromMatches(self):
+        # Setup artifical matches that correspond to a known (random) PolynomialTransform.
+        order = 3


I suggest moving this code into a new method, e.g. makeMatchList, to make the test easier to read

I'd rather not; it'd only be used in one place, and I think the comments are sufficient to make it clear where in the code we're doing prep work rather than doing the tests. And I don't want to comb through this function to find which variables I set in the top half I'll need to pass out of the function unless there's a very good reason to split them up.

r-owen · 2016-10-18T20:47:47Z

tests/testTransforms.py

+        def t2(p):
+            sky = lsst.afw.coord.IcrsCoord(crval + lsst.afw.geom.Extent2D(p), lsst.afw.geom.degrees)
+            return wcs.skyToPixel(sky)
+        self.assertTransformsNearlyEqual(t1, t2)


I had asked if it was also worth copying this test for the reverse direction (using different words) and you replied "That's actually a lot harder - it requires actually inverting the polynomial part of rev (note that fwd isn't actually the inverse of rev right now). I do essentially that down in testFromGrid."

For the record, all I had in mind was literally testing rev the same way you test fwd, not testing them against each other (I realize they are not inverses of each other, since your documentation was clear on that point). I don't know if it would actually increase test coverage.

Ah, now I understand. I think that's a good idea, and I started doing it - but to do it it looks like I'd need a Wcs method that doesn't exist: an inverse of skyTointermediateWorldCoord. (And now that I've said that, I can vaguely recall coming to the same conclusion back when I originally wrote this test).

That makes sense. May I suggest you add a small comment to that effect?

fred3m · 2016-10-19T20:33:57Z

include/lsst/meas/astrom/ScaledPolynomialTransformFitter.h

+ *     scatter estimation.
+ *
+ *  In either case, the fitter creates affine transforms that map the input
+ *  and output data points onto to [-1, 1].  In then fits a polynomial


Typo: This should be "It then fits a polynomial...".

This allows for better debugging displays.

TallJimbo force-pushed the tickets/DM-3549 branch from bf8b9e7 to 6337b7f Compare October 4, 2016 17:23

r-owen reviewed Oct 6, 2016

View reviewed changes

pschella reviewed Oct 6, 2016

View reviewed changes

r-owen reviewed Oct 6, 2016

View reviewed changes

r-owen reviewed Oct 7, 2016

View reviewed changes

r-owen reviewed Oct 18, 2016

View reviewed changes

fred3m reviewed Oct 19, 2016

View reviewed changes

TallJimbo force-pushed the tickets/DM-3549 branch from 66a6bf5 to f5e4806 Compare October 26, 2016 18:38

TallJimbo added 8 commits October 26, 2016 17:15

C++ classes and Swig for new SIP fitter.

d9bbaf5

Add optional exposure argument to WCS fitters.

5ca391c

This allows for better debugging displays.

Add logging to old FitTanSipWcsTask to report outliers rejected.

56f311d

Add new Python task for SIP distortion fitting.

557ed8e

Switch sign and name for outlier-rejection field.

5e5500e

Adapt to move of ndarray/swig files.

5c7dcce

Split Swig for transform classes into a separate file.

a673f10

Make frame and pausing configurable in FitSipDistortionTask displays.

9286798

TallJimbo force-pushed the tickets/DM-3549 branch from f5e4806 to 9286798 Compare October 26, 2016 21:16

TallJimbo merged commit b143333 into master Oct 26, 2016

ktlim deleted the tickets/DM-3549 branch August 25, 2018 06:15


		namespace lsst { namespace meas { namespace astrom { namespace detail {

		inline int computePackedOffset(int order) {


		%include "lsst/meas/astrom/matchOptimisticB.h"

		%include "std_pair.i"

		poly = makeRandomPolynomialTransform(order, sip=False)
		return SipReverseTransform(origin, cd, poly)


		class TransformTestMixin(object):

		def checkTransformEquivalence(self, a, b, rtol=1E-8):

meas_astrom/DM-3549: new SIP fitter #47

meas_astrom/DM-3549: new SIP fitter #47

Conversation

TallJimbo commented Oct 3, 2016

r-owen left a comment

Choose a reason for hiding this comment

r-owen Oct 5, 2016 • 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

r-owen Oct 5, 2016 • 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

r-owen Oct 5, 2016 • 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

r-owen Oct 6, 2016 • 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

r-owen Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen left a comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • 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 Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • 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

r-owen Oct 6, 2016 • 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

Choose a reason for hiding this comment

TallJimbo Oct 7, 2016 • edited

Choose a reason for hiding this comment

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • 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

Choose a reason for hiding this comment

r-owen Oct 6, 2016 • edited

r-owen Oct 5, 2016 •

edited

r-owen Oct 5, 2016 •

edited

r-owen Oct 5, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited

TallJimbo Oct 7, 2016 •

edited

r-owen Oct 6, 2016 •

edited

r-owen Oct 6, 2016 •

edited