DM-10486: warpExposure and warpImage do not test correctly for dest = src #352

r-owen · 2018-05-10T00:22:37Z

No description provided.

kfindeisen

imagesOverlap's behavior is compiler-dependent, and we can't fix that entirely, but mitigating it is pretty easy. Otherwise looks good.

kfindeisen · 2018-05-10T17:26:35Z

include/lsst/afw/image/MaskedImage.h

+/**
+ * Return true if the pixels for two masked images (image, variance or mask plane) overlap in memory
+ * (e.g. one is identical to or a subimage of the other).
+ */


Don't the planes of a MaskedImage need to correspond to each other? Shouldn't either all overlap, or none of them overlap?

It turns out to be trivial to make a MaskedImage from any Image, Mask and Variance, so long as they have the same dimensions (and perhaps the same XY0). Just call lsst.afw.image.makeMaskedImage. I added a unit test that does exactly that.

It is not very likely that real pipeline code would produce such masked images, but we cannot rule it out and it's quick and easy to test for this case.

Oh, I agree the code is fine. I was only asking about the wording of the documentation.

Feel free to suggest other wording.

Note that it is, in theory, possible for planes (image, variance, mask) to actually overlap in the sense that the union is a subset of both (e.g. the way [0, 5] and [2, 7] overlap). Constructing such images takes some work (probably in ndarray or numpy) and I don't expect anyone will do it. But if they do, this test will, in theory, catch it.

I'll remove the parenthetical comment

kfindeisen · 2018-05-10T17:30:03Z

python/lsst/afw/image/image/image.cc

@@ -341,6 +341,9 @@ PYBIND11_PLUGIN(image) {
    declareCastConstructor<std::uint64_t, float>(clsImageF);
    declareCastConstructor<std::uint64_t, double>(clsImageD);

+    // Note: wrap both the Image and MaskedImage versions of imagesOverlap in the MaskedImage wrapper,
+    // as wrapping the Image version here results in it being invisible


I assume this is because of a name collision in the afw.image package, and that they're still visible in afw.image.image and afw.image.maskedImage? If so, it might be good to say that the key is to keep all overloads in the same Python module.

That sounds plausible but I did not test it. I will add "invisible in lsst.afw.image"

kfindeisen · 2018-05-10T17:38:54Z

python/lsst/afw/image/maskedImage/maskedImage.cc

+    declareImagesOverlap<std::uint64_t, float>(mod);
+    declareImagesOverlap<std::uint64_t, double>(mod);
+    declareImagesOverlap<std::uint64_t, std::uint16_t>(mod);
+    declareImagesOverlap<std::uint64_t, std::uint64_t>(mod);


I wonder if it's possible to write something that uses TMP to deal with these "outer product" style wrappers. I think we're using them in quite a few places at this point.

I agree that would be very nice.

kfindeisen · 2018-05-10T18:10:34Z

src/image/Image.cc

+
+    auto arr1 = image1.getArray();
+    auto beg1Addr = reinterpret_cast<void const *>(arr1.front().begin());
+    auto end1Addr = reinterpret_cast<void const *>(arr1.back().end());


Ugh, I had to dig deep into both ndarray and the C++ language spec to figure out what this code does. I'll concede that you do need to compare void* rather than T1* and T2*, but I suggest getting rid of the reinterpret_cast. Any pointer can be implicitly converted to void*, so the use of a frequently unsafe cast just makes things confusing (especially since it's not at all obvious to the reader that arr1.front().begin() is a T1*).

I thought I needed reinterpret_cast but you are right that I can do without it (the compiler was complaining about something else).

I agree that arr1.front().begin() is less than obvious, but it's what @TallJimbo suggested and I think it's safer than arr[height][width] because one can easily get the indices in the wrong order. I will document what it does, but I can't think of anything else to do about it.

I think if you use an implicit conversion to void* than there's no need to document that construct -- pointer to element is the only thing that both compiles and makes sense as an iterator type.

kfindeisen · 2018-05-10T18:51:11Z

src/image/Image.cc

+    auto beg2Addr = reinterpret_cast<void const *>(arr2.front().begin());
+    auto end2Addr = reinterpret_cast<void const *>(arr2.back().end());
+
+    return beg1Addr < end2Addr && beg2Addr < end1Addr;


According to cppreference, the < operator has undefined behavior for two pointers to unrelated variables; it's not even required to be self-consistent(!). However, std::less is safe.

Let's see... beg1Addr < end1Addr and beg2Addr < end2Addr are guaranteed because they're from the same array. The order also works the way you'd intuitively expect if arr1 and arr2 are backed by the same C array, so there will be no false negatives. However, while std::less<void*> guarantees a strict total order, it does not guarantee that order is globally consistent with memory address numbers. I think that means that, strictly speaking, any overlap test based on pointer comparisons may have false positives. 😞

That sounds like a fatal flaw in any such test. Is it even safe to compare pointer identity? If so, I could at least check that one is not warping an exposure into self. If not, it may be time to give up on the test and just say "don't do that".

I wonder if @TallJimbo will have an opinion on this.

You're right, the documentation applies to == even though that's a common idiom. Let's hope I've misunderstood something. 😨

kfindeisen · 2018-05-10T19:57:45Z

tests/test_maskedImage.py

+            afwGeom.Box2I(afwGeom.Point2I(0, 0), afwGeom.Extent2I(6, 5)),
+            afwGeom.Box2I(afwGeom.Point2I(2, 1), afwGeom.Extent2I(6, 5)),
+            afwGeom.Box2I(afwGeom.Point2I(4, 3), afwGeom.Extent2I(6, 5)),
+        )


Again, consider including non-overlapping boxes.

kfindeisen · 2018-05-10T19:58:51Z

tests/test_maskedImage.py

+                                                              mask=afwImage.Mask(mask2, bbox2),
+                                                              variance=afwImage.ImageF(variance2, bbox2))
+                    self.assertEqual(afwImage.imagesOverlap(subMi1, subMi2), predOverlap)
+                    self.assertEqual(afwImage.imagesOverlap(subMi2, subMi1), predOverlap)


Should these two assertions be inside the loop?

kfindeisen · 2018-05-10T19:59:38Z

tests/test_warpExposure.py

-            self.fail(
-                "warping into a destination Exception with no Wcs should fail")
-        except Exception:
-            pass


kfindeisen · 2018-05-10T20:03:04Z

tests/test_maskedImage.py

@@ -149,6 +150,55 @@ def testSetGetValues(self):
        self.assertEqual(self.mimage.getMask().get(1, 1), self.EDGE)
        self.assertEqual(self.mimage.getMask().get(2, 2), 0x0)

+    def testImagesOverlap(self):


There's a lot of code duplication with test_images.py. Can't some of it (e.g., the bounding box set and the associated subtest) be factored out?

Probably, I'm just not sure where I'd put it. I'm inclined to leave it as "good enough".

kfindeisen · 2018-05-10T20:06:01Z

include/lsst/afw/image/Image.h

+ * Return true if the pixels for two images or masks overlap in memory
+ *
+ * Images are considered to overlap if they are identical or one is a
+ * subimage of the other.


What is the desired behavior if two images share pixels, but both have unshared pixels as well? I think this can happen if you have two subimages of the same image.

Return true. I'll update the documentation to try to clarify this.

Actually I think the simplest thing to do is get rid of the longer explanation and just leave the summary.

kfindeisen · 2018-05-10T23:35:35Z

src/image/Image.cc

+template <typename A, typename B, typename PtrLess = std::less<void const*>>
+bool ptrsOverlap(A beg1Addr, A end1Addr, B beg2Addr, B end2Addr, PtrLess ptrLess = PtrLess()) {
+    return ptrLess(beg1Addr, end2Addr) && ptrLess(beg2Addr, end1Addr);
+}


I think this could be simplified a bit:

bool ptrsOverlap(void const* const beg1Addr, void const* const end1Addr, void const* const beg2Addr, void const* const end2Addr) { static std::less<void const*> const ptrLess; // std::less is safer than < for generic pointers return ptrLess(beg1Addr, end2Addr) && ptrLess(beg2Addr, end1Addr); }

(depending on how rigorous you want to get with const, of course).

with overloads for images and masks (both based on ImageBase) and masked images.

Make warpImage call imagesOverlap instead of isSameObject (which didn't compare the right thing and didn't even try to detect overlap). Document that the source and destination must not overlap (formerly we only attempted to make sure they were not the same image). Fix the unit test that checked that overlapping images could not be warped onto each other. It was completely broken.

from afw::math::details

Catch lsst.pex.exceptions.InvalidParameterError instead of Exception (in some cases) or lsst.pex.exceptions.Exception (in others)

r-owen force-pushed the tickets/DM-10486 branch from 45e7b7f to f8304a7 Compare May 10, 2018 00:28

kfindeisen approved these changes May 10, 2018

View reviewed changes

kfindeisen reviewed May 10, 2018

View reviewed changes

r-owen force-pushed the tickets/DM-10486 branch from e2055c6 to bda3f5e Compare May 11, 2018 20:51

git hide .pytest_cache

4400feb

r-owen force-pushed the tickets/DM-10486 branch from 405a8d4 to 9131049 Compare May 14, 2018 19:49

Add lsst::afw::image::imagesOverlap function

812369c

with overloads for images and masks (both based on ImageBase) and masked images.

r-owen force-pushed the tickets/DM-10486 branch from 9131049 to f4d877b Compare May 14, 2018 19:50

r-owen added 3 commits May 14, 2018 12:51

Remove obsolete function isSameObject

f2f22fb

from afw::math::details

Tighten some asserts in test_warpExposure.py

473a8d6

Catch lsst.pex.exceptions.InvalidParameterError instead of Exception (in some cases) or lsst.pex.exceptions.Exception (in others)

r-owen force-pushed the tickets/DM-10486 branch from f4d877b to 473a8d6 Compare May 14, 2018 19:52

r-owen merged commit 473a8d6 into master May 14, 2018

ktlim deleted the tickets/DM-10486 branch August 25, 2018 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10486: warpExposure and warpImage do not test correctly for dest = src #352

DM-10486: warpExposure and warpImage do not test correctly for dest = src #352

r-owen commented May 10, 2018

kfindeisen left a comment

kfindeisen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018 •

edited

kfindeisen May 10, 2018

r-owen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

kfindeisen May 10, 2018

kfindeisen May 10, 2018

kfindeisen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018

r-owen May 10, 2018

r-owen May 10, 2018

kfindeisen May 10, 2018 •

edited

DM-10486: warpExposure and warpImage do not test correctly for dest = src #352

DM-10486: warpExposure and warpImage do not test correctly for dest = src #352

Conversation

r-owen commented May 10, 2018

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

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

kfindeisen May 10, 2018 • 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

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

kfindeisen May 10, 2018 • edited

Choose a reason for hiding this comment

kfindeisen May 10, 2018 •

edited

kfindeisen May 10, 2018 •

edited