DM-14527: change origin for image accessors #369
Conversation
Avoids overload conflict with setting the full image to scalar.
There was a problem hiding this comment.
For the most part everything looks good. I think there are a few extra _get and _set methods that are unnecessary, but I like that you placed a number of functions in slicing.py that I can use for the multiband classes (to ensure consistency).
The only real API change I recommend is allowing negative indexing for images with origin = PARENT as long as XY0 > (0,0). See my comments below.
I also recommend removing a lot of the afwImage.PARENT arguments in the tests, since that is the default value and should work as expected without them.
|
|
||
| .. currentmodule:: lsst.afw.image | ||
|
|
||
| LSST's image classes (`Image`, `Mask`, `MaskedImage`, and `Exposure`) use a pixel indexing convention that is different from both the convention used by `numpy.ndarray` objects and the convention used in FITS images. |
There was a problem hiding this comment.
I would make this and the next paragraph a warning to catch the readers attention.
There was a problem hiding this comment.
Given that it's the first paragraph and the first thing readers on this page are going to see anyway, I think I disagree; warnings in docs are usually an aside, not the main text.
There was a problem hiding this comment.
Perhaps, but I feel like there should be a warning to catch the careless reader's attention. Most of the "bug" reports that we've received for scarlet are people misinterpreting the coordinate convention, and as an often careless reader myself, I like it when people go out of their way to highlight potentially confusing conventions.
There was a problem hiding this comment.
Yeah, maybe it'd work best if I added a warning box with new text, rather than stealing the old text for that.
| >>> print(sub1.getBBox(LOCAL)) | ||
| Box2I(minimum=Point2I(0, 0), dimensions=Extent2I(6, 7)) | ||
|
|
||
| Note that the `PARENT` bounding box's minimum point is ``xy0``, while the `PARENT` bounding box's minimum point is ``(0, 0)``; this is *always* true. |
There was a problem hiding this comment.
This might want to be an actual .. note::
| As in the previous case, when we make a subimage using a box in `PARENT`coordinates, the PARENT bounding box of the result is that same box. | ||
| When we make a subimage using a box in `LOCAL` coordinates, that input box is different from both the resulting subimage's `LOCAL` bounding box and its `PARENT` bounding box. | ||
|
|
||
| We strongly recommend using the `PARENT` convention whenever possible (which usually just means not explicitly selecting `LOCAL`, of course, since `PARENT` is the default). |
| }}}} // namespace lsst::afw::image::python | ||
|
|
||
| #endif //! LSST_AFW_IMAGE_PYTHON_INDEXING_H | ||
|
|
There was a problem hiding this comment.
I'm not sure why this has to be implemented in C++. Wouldn't it be easier (and more straightforward) to just implement this method in python, or at the very least in the pybind11 wrapper?
There was a problem hiding this comment.
It is actually only used in the pybind11 wrappers (its in a header because it's used by multiple pybind11 wrappers), and it's there rather than in Python so we don't need to have both an unsafe _ method defined in pybind11 and a safe non-_ method defined in pure Python.
|
|
||
| cls.def("set", [](ImageBase<PixelT> &img, PixelT val) { img = val; }); | ||
| cls.def( | ||
| "_set", |
There was a problem hiding this comment.
I feel like _get and _set are unnecessary, since they could both be implemented as a single line in __getitem__ and __setitem__ in slicing.py or subset could be overloaded to take a Point2I and return a scalar.
There was a problem hiding this comment.
The issue is that _get and _set are implemented differently for different types; they provide the common interface that allows slicing.py to not specialize for Image vs. MaskedImage vs. Exposure.
There was a problem hiding this comment.
Right. I knew that was true for Exposure, which is enough to warrant a special method.
tests/test_image.py
Outdated
| @@ -69,7 +69,7 @@ def makeRampImage(width, height, imgClass=afwImage.ImageF): | |||
| val = 0 | |||
| for yInd in range(height): | |||
| for xInd in range(width): | |||
| im.set(xInd, yInd, val) | |||
| im[xInd, yInd, afwImage.PARENT] = val | |||
There was a problem hiding this comment.
see comment in test_background.py. There's no need to include the origin since PARENT is the default value.
tests/test_mask.py
Outdated
| @@ -140,10 +140,10 @@ def testArrays(self): | |||
| def testInitializeMasks(self): | |||
| val = 0x1234 | |||
| msk = afwImage.Mask(lsst.geom.ExtentI(10, 10), val) | |||
| self.assertEqual(msk.get(0, 0), val) | |||
| self.assertEqual(msk[0, 0, afwImage.PARENT], val) | |||
There was a problem hiding this comment.
see comment in test_background.py. There's no need to include the origin since PARENT is the default value.
tests/test_imageIo2.py
Outdated
| @@ -54,7 +54,7 @@ def testIo(self): | |||
| image = Image(self.cols, self.rows) | |||
| for x in range(0, self.cols): | |||
| for y in range(0, self.rows): | |||
| image.set(x, y, x + y) | |||
| image[x, y, afwImage.PARENT] = x + y | |||
There was a problem hiding this comment.
see comment in test_background.py. There's no need to include the origin since PARENT is the default value.
tests/test_mask.py
Outdated
| @@ -140,10 +140,10 @@ def testArrays(self): | |||
| def testInitializeMasks(self): | |||
| val = 0x1234 | |||
| msk = afwImage.Mask(lsst.geom.ExtentI(10, 10), val) | |||
| self.assertEqual(msk.get(0, 0), val) | |||
| self.assertEqual(msk[0, 0, afwImage.PARENT], val) | |||
There was a problem hiding this comment.
see comment in test_background.py. There's no need to include the origin since PARENT is the default value.
tests/test_methods.py
Outdated
| @@ -145,7 +145,7 @@ def checkMaskedImage(self, mi): | |||
| mi1 = mi.Factory(mi, True) | |||
| plane0 = getattr(mi0, getName)() | |||
| plane1 = getattr(mi1, getName)() | |||
| plane1[2, 2] = errVal | |||
| plane1[2, 2, afwImage.PARENT] = errVal | |||
There was a problem hiding this comment.
see comment in test_background.py. There's no need to include the origin since PARENT is the default value.
TallJimbo
force-pushed
the
tickets/DM-14527
branch
from
1edad8f
to
afc9fb9
Compare
|
Looks good, but it might be useful to update the docs regarding negative indices before merging. |
TallJimbo
force-pushed
the
tickets/DM-14527
branch
from
39999d5
to
0145688
Compare
Just prior to this commit, the default was NotImplemented, which was intended to break code that did not provide an origin in order ensure it was fixed. All such code should have now been found.
TallJimbo
force-pushed
the
tickets/DM-14527
branch
from
0145688
to
75313a3
Compare
|
Feel free to bring me into future documentation PRs for style feedback. |
No description provided.