DM-15431: Add PixelScaleBoundedField #411

parejkoj · 2018-10-31T02:33:18Z

A bounded field to compute the effective pixel size due to astrometric
distortions, used to convert fluxes between surface brightness and fluence.

PaulPrice

Several minor comments to be addressed before merging.

PaulPrice · 2018-11-05T19:39:59Z

include/lsst/afw/math/PixelScaleBoundedField.h

+
+    /// Get the contained SkyWcs
+    geom::SkyWcs const &getSkyWcs() const { return _skyWcs; }
+    // Get the cached inverse pixel scale


PaulPrice · 2018-11-05T19:40:42Z

include/lsst/afw/math/PixelScaleBoundedField.h

+
+    // std::string getPythonModule() const override;
+
+    // void write(OutputArchiveHandle &handle) const override;


Commented lines should be removed.

How is the persistence done if these are commented out?
And the persistence isn't tested.

PixelScaleBoundedField is not persistable: isPersistable() is false See comments on the ticket: it will likely only be used in-memory, and is trivially constructable from a SkyWcs.

So is it necessary to inherit from table::io::PersistableFacade? If so, there should be a comment explaining why.

PaulPrice · 2018-11-05T19:42:59Z

include/lsst/afw/math/PixelScaleBoundedField.h

+    /// Get the contained SkyWcs
+    geom::SkyWcs const &getSkyWcs() const { return _skyWcs; }
+    // Get the cached inverse pixel scale
+    double getDefaultInverseScale() const { return _inverseScale; }


Why is Default included in the name?

PaulPrice · 2018-11-05T19:43:45Z

include/lsst/afw/math/PixelScaleBoundedField.h

+    // Inverse pixel scale (square degrees), for convenience.
+    double const _inverseScale;
+
+    std::string toString() const override { return ""; }


I don't think this is a good idea. Either remove this method, or implement it properly.

Right, I was trying to think of what to put in there, but I think I'll just leave it unimplemented, since I can't come up with a good str representation.

I forgot this was pure virtual: I put in a trivial implementation.

Should at least return the class name, and the WCS pointer address might be useful for debugging if we can't come up with anything better.

The updated version does that.

PaulPrice · 2018-11-05T19:45:24Z

python/lsst/afw/math/pixelScaleBoundedField.cc

+
+#include "lsst/afw/geom/SkyWcs.h"
+#include "lsst/afw/math/BoundedField.h"
+#include "lsst/afw/math/PixelScaleBoundedField.h"


PixelScaleBoundedField.h already includes the other two.

Are there guidelines on what should/shouldn't be listed in the #include list? I was going with "explicit is better than implicit", but that's a python quote, so...

I don't know if there are firm guidelines, but I generally try to keep the number of includes to a minimum. In this case it's just a distraction because the include guards should keep these extra includes from causing any trouble, but in general minimising the number of includes minimises the compile time.

PaulPrice · 2018-11-05T19:46:56Z

python/lsst/afw/math/pixelScaleBoundedField.cc

+
+#include "ndarray/pybind11.h"
+
+#include "lsst/pex/config/python.h"    // defines LSST_DECLARE_CONTROL_FIELD


I don't think you need the last three includes.

I wish there was a warning to tell you that you had unused includes.

PaulPrice · 2018-11-05T19:48:26Z

python/lsst/afw/math/pixelScaleBoundedField.cc

+
+    cls.def("__repr__", [](PixelScaleBoundedField const &self) {
+        std::ostringstream os;
+        os << "PixelScaleBoundedField(" << self << ")";


How does this work?

What do you mean? It adds the name of the object to the operator<< output of the object, per our guidelines.

Where does the operator<< output of the object come from? I don't see it implemented.

BoundedField (parent class) itself (BoundedField.h:203).

PaulPrice · 2018-11-05T19:53:19Z

tests/test_pixelScaleBoundedField.py

+        expect = np.zeros(len(points), dtype=float)
+        for i, point in enumerate(points):
+            scale = self.skyWcs.getPixelScale(lsst.geom.Point2D(point)).asDegrees()
+            expect[i] = scale*scale / (self.skyWcs.getPixelScale().asDegrees())**2


I don't think you should use the exact same implementation in the test as in the source code: it won't discover any errors that are due to changes in the SkyWcs class. You should use an analytic solution for the comparison (as you say, the WCS is "trivial").
Another suggestion: use two WCSes with different scales, divide one field by the other and you should get the square of the ratio of the scales.

I'm not sure I understand what you're asking. The goal here isn't to test whether SkyWcs is calculating pixel scale correctly (it has its own tests for that), so re-implementing some pixel area code seems excessive.

Having the rather trivial test as-written helped me sort out some bugs in development, related to unit conversions.

I observe that the implementation here is exactly the same as it is in the source. It seems to me that this, while not completely useless (as you've pointed out), is not sufficient for securing the code against upstream changes. It seems like a waste of code to me, and I would prefer to remove the test completely rather than just have a test that will always pass because it does the exact same thing as what's being tested.

It's not "exactly the same": it's python instead of C++, which is often the only thing you can do for a test of mathematical functions.

What upstream changes are you afraid of?

We may also need/want to implement a vectorized version of the array evaluation code, which would require this existing python test code to pass.

I do think what's here is better than nothing, as I do expect us to want to change the C++ implementation in the future, and in the meantime at least we're running the code and verifying that it doesn't throw or produce garbage.

PaulPrice · 2018-11-05T20:00:50Z

tests/test_pixelScaleBoundedField.py

+        self.assertFloatsAlmostEqual(expect, result)
+
+    def testEvaluateArray(self):
+        expect = self._computeExpected(self.points)


Can't you create an image of the entire bbox, rather than just testing a few points?

I've made a grid of points with np.meshgrid and evaluated that.

Why isn't there an API in BoundedField to produce such an image?

What do you mean? BoundedField has nothing to do with creating images.

There's boundedField.fillImage(), but you still have to create the image first.

BoundedField is a field defined over a Box2I. It's very natural to ask for an image of the field, so I'm surprised that such an API doesn't exist. Here's a suggested implementation:

template <typename T> image::Image<T> constructImage() const { auto image = std::make_shared<image::Image<T>>(_bbox); fillImage(*image); return image; }

@TallJimbo : do you want to weigh-in on this?

That would be a reasonable addition, and small enough that it could reasonably be added here instead of on another ticket (albeit with the optional xStep and yStep arguments) unless there's a lot of time pressure (which may be the case).

Blame for it not existing already goes to me, of course, not @parejkoj, and I don't recall my reasoning; may have been an oversight or a paranoid attempt to discourage unnecessary image copies along the lines of of Image lacking __add__.

PaulPrice · 2018-11-05T20:02:15Z

tests/test_pixelScaleBoundedField.py

+
+    def testString(self):
+        # TODO: anything else to put in a str or repr?
+        self.assertIn("PixelScaleBoundedField(", repr(self.boundedField))


I think this needs to be more complete.

There's nothing else to test, given that there's no str implementation.

I don't understand why we're testing a not-implemented method at all.

I also think an implementation of str that writes the pointer address of the internal SkyWcs is probably more useful than none at all (pity that SkyWcs doesn't provide any concise stringification), though I'm also not sure that is worth a test case.

A bounded field to compute the effective pixel size due to astrometric distortions, used to convert fluxes from surface brightness to fluence.

parejkoj force-pushed the tickets/DM-15431 branch 2 times, most recently from 1593a5a to 36232ea Compare October 31, 2018 06:16

PaulPrice approved these changes Nov 5, 2018

View reviewed changes

parejkoj force-pushed the tickets/DM-15431 branch from 36232ea to 98820f3 Compare November 6, 2018 00:34

Add PixelScaleBoundedField

a8d6d7b

A bounded field to compute the effective pixel size due to astrometric distortions, used to convert fluxes from surface brightness to fluence.

parejkoj force-pushed the tickets/DM-15431 branch from 98820f3 to a8d6d7b Compare November 7, 2018 21:19

parejkoj merged commit 8d4b44b into master Nov 7, 2018

timj deleted the tickets/DM-15431 branch February 18, 2021 00:01


		// std::string getPythonModule() const override;

		// void write(OutputArchiveHandle &handle) const override;


		#include "ndarray/pybind11.h"

		#include "lsst/pex/config/python.h" // defines LSST_DECLARE_CONTROL_FIELD

DM-15431: Add PixelScaleBoundedField #411

DM-15431: Add PixelScaleBoundedField #411

Conversation

parejkoj commented Oct 31, 2018 • edited Loading

PaulPrice 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

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

parejkoj commented Oct 31, 2018 •

edited

Loading