tickets/DM-13911: Create multiband classes #353

fred3m · 2018-05-10T14:48:58Z

No description provided.

TallJimbo · 2018-05-11T14:58:52Z

python/lsst/afw/image/multiband.py

+        a `_singles` attribute that is a list of single band objects,
+        a `_filters` attribute that is a list of filter
+        names, a `_bbox` attribute is a `Box2I`,
+        and a `_xy0` attribute that is a `Point2I`.


I don't think you need both of these; _bbox.getMin() should always be equivalent to _xy0. This also applies to all of the attributes and properties relating to xy0; those should all delegate to the bbox, so you only have it in one place.

Good point, I'll remove _xy0

TallJimbo · 2018-05-11T15:00:41Z

python/lsst/afw/image/multiband.py

+            Slices used to extract a subset of an image
+            (used when `singles` inherits from `MultibandBase` to
+            create a new object with a slice of the original).
+        """


If the base class __init__ is being passed singles, shouldn't it be responsible for creating the _singles attribute from it?

On that note, it'd probably be cleaner to have the base __init__ also take bbox and filters and create those attributes as well, rather than requiring derived __init__ to create any attributes used by the base class.

You should then remove the exception raise below and expect derived class __init__ to call base class __init__.

TallJimbo · 2018-05-11T15:05:38Z

python/lsst/afw/image/multiband.py

+
+    @property
+    def singles(self):
+        """List of afw single band objects


Better return a tuple, so it's impossible to do obj.singles.append(another).

TallJimbo · 2018-05-11T15:08:45Z

python/lsst/afw/image/multiband.py

+        return self._singles
+
+    @property
+    def bbox(self):


I'm afraid bbox and xy0 don't meet our requirements for properties (https://developer.lsst.io/python/style.html#properties-should-be-used-when-they-behave-like-regular-instance-attributes), because they return mutable objects that can be modified with surprising results. In other words, you can do

obj.bbox.grow(3)

or

obj.xy0.setX(4)

And end up with an inconsistent object. Our convention would be to just define getters and setters for these, since it's much less likely that someone would expect e.g. obj.getBBox().grow(3) to affect obj.

TallJimbo · 2018-05-11T15:12:17Z

python/lsst/afw/image/multiband.py

+    def yx0(self):
+        """Minimum (y,x) position
+        """
+        return (self.y0, self.x0)


I'm not a huge fan of having a reversed version of xy0 in this form:

the symmetry with xy0 in terms of naming would imply that it returns some kind of point object

anything defined here should also be available on single-band Image/MaskedImage/Exposure objects.

I'm guessing you want it to offset indices into numpy array views? If so, I'm not opposed to having something that does this, but I'd like a name that suggests its purpose ("arrayOrigin"?) rather than symmetry with xy0, and whatever you go with should be added to the single-band classes.

TallJimbo · 2018-05-11T15:19:32Z

python/lsst/afw/image/multiband.py

+
+    def __repr__(self):
+        result = "<{0}, filters={1}, yx0={2}, image shape={3}>".format(
+            self.__class__.__name__, self.filters, self.yx0, self.array.shape[-2:])


Suggest showing the bbox here instead of yx0 and shape; LSST convention is (x, y) ordering, even if we need to support the transposed version for NumPy interoperability in some places.

TallJimbo · 2018-05-11T15:21:09Z

python/lsst/afw/image/multiband.py

+    def array(self):
+        """Data cube array in multiple bands
+        """
+        return self._array


Should add a setter for this property; could be implemented just as:

@array.setter def _setArray(self, value): self.array[:, :] = value

TallJimbo · 2018-05-11T15:21:29Z

python/lsst/afw/image/multiband.py

+    def shape(self):
+        """Shape of the Multiband Object
+        """
+        return self.array.shape


I'd remove this; requiring the user to call obj.array.shape is a bit more verbose but much clearer, and it's another place where I think it'd be confusing to have a method on a multi-band class but not its single-band counterpart.

TallJimbo · 2018-05-11T15:23:24Z

python/lsst/afw/image/multiband.py

+                    if yslice.start is not None:
+                        y0 += yslice.start
+                else:
+                    raise IndexError("y index must be a slice")


Please make sure this works with box slices as well.

TallJimbo · 2018-05-11T15:24:08Z

python/lsst/afw/image/multiband.py

+            # whose data points to the appropriate slice
+            # of self.array
+            self._updateSingles(singles.imageType)
+        else:


I'd recommend splitting this up into at least two private methods called by __init__ for clarity.

TallJimbo

Everything looks good from a large-scale structural standpoint, but I'd like to see some refactoring to reduce complexity and maintenance load:

I'd like to see most constructors split up into a simpler constructor and one or more static/class methods - I feel like they're simply trying to process too many mostly-orthogonal arguments at once.
There are some methods (mostly bbox setters) that I suspect we could do without, and perhaps even some classes (the single-pixel ones). While I know you've already done a lot of work implementing those, so it may not seem keeping them does much harm, I'm worried that the maintenance load could be larger than any future convenience they might provide. I am of course quite happy to be convinced they should be kept if they are things you have direct plans to use.

TallJimbo · 2018-06-29T18:22:45Z

python/lsst/afw/detection/__init__.py

@@ -23,3 +23,4 @@
 """Application Framework detection-related classes including Source
 """
 from .detectionLib import *
+from .multiband import *


Add newline.

TallJimbo · 2018-06-29T18:25:54Z

python/lsst/afw/multiband.py

+            List of filter names.
+        singles: list
+            List of single band objects
+        """


__init__ documentation should go in the class docstring (https://developer.lsst.io/python/numpydoc.html#placement-of-class-docstrings)

TallJimbo · 2018-06-29T18:26:45Z

python/lsst/afw/multiband.py

+        """Initialize a `MultibandBase` object
+
+        Must be overloaded in derived classes to use `array` and slicing
+        functionality.


Should say a bit more about what derived class __init__ methods must do to use that functionality, and whether they're permitted not to.

Should also probably say that derived classes must call the base class __init__.

This was an outdated comment, thanks for pointing this out. I also clarified the docs that the source must either call the base class __init__ or set the _filters, _singles, _bbox, and _singleType properties.

TallJimbo · 2018-06-29T19:04:07Z

python/lsst/afw/multiband.py

+            copy of the instance that inherits from `MultibandBase`
+        """
+        err = "_copySingles must be implemented in an inherited class to enable copies"
+        raise NotImplementedError(err)


Have you considered using collections.ABC and abstractmethod here?

I hadn't, but this is a good idea and is now implemented (and also helped me catch that I didn't implement copy for MultibandFootprint).

TallJimbo · 2018-06-29T19:05:02Z

python/lsst/afw/multiband.py

+
+    @property
+    def singles(self):
+        """List of afw single band objects


Probably shouldn't say "afw" here, as subclasses of MultibandBase don't have to use afw objects, even if all current ones do.

TallJimbo · 2018-06-29T22:49:02Z

python/lsst/afw/image/utils.py

+    -------
+    targetSlices, imageIndices: tuple, tuple
+        Slices of the target and input images
+        in the form (by, bx), (iy, ix).


I think this will only be parsed correctly by Sphinx/Numpydoc if you split it into multiple lines.

TallJimbo · 2018-06-29T22:49:39Z

python/lsst/afw/image/utils.py

+    -------
+    newImage: `afw.Image` or `afw.MaskedImage`
+        The new image with the input image projected
+        into it's bounding box.


it's -> its.

TallJimbo · 2018-06-29T22:51:20Z

python/lsst/afw/image/utils.py

+    """Project an image into a bounding box
+
+    Project an image into a new image with a
+    (potentially) different bounding box.


"Project" isn't quite clear enough of a verb to explain what this function does for the more detailed description, I think.

I think you want something like "Return a new image whose pixels are equal to those of image within bbox, and equal to fill outside".

TallJimbo · 2018-06-29T23:16:27Z

tests/test_multiband.py

+from lsst.afw.image.multiband import MaskedPixel, MultibandMaskedPixel, MultibandExposure
+
+
+def _testImageFilterSlicing(cls, mImage, singleType, bbox, value):


I think this cls should be self, or maybe something indicating that it's a TestCase instance, not a type object of some kind. Might be best to put these helper methods in a mixin class or two.

At least one-line docstrings describing what kinds of operations are tested would be welcome as well.

(These comments of course apply to the test helpers below as well).

I thought that the typical syntax for a function in python that takes a class as the first argument is to use cls as opposed to self. I did add the docstrings as requested.

Ah, I was thinking that you were passing in the TestCase instance as the first argument. The fact that you aren't also explains the use of vanilla asserts. So this is indeed okay, though it might be even better to make these TestCase methods so you can use asserts that produce better error messages.

Sorry, I explained this wrong and now realize why I need to rename them. These are instances of TestCases, so I should probably rename cls to testCase.

TallJimbo · 2018-06-29T23:16:59Z

tests/test_multiband.py

+    assert isinstance(mImage["R"], singleType)
+    assert isinstance(mImage[:], type(mImage))
+    idx = np.int32(2)
+    assert isinstance(mImage[idx], singleType)


I think these lines should be cls.assertIsInstance(..., ...).

TallJimbo · 2018-07-06T19:47:04Z

@fred3m, I'm about to get on an airplane, and I don't think there's anything else controversial to discuss right now, so I'm approving changes and you should feel welcome to merge after you things are in good shape. Of course, I'm happy to take another look if you aren't in a rush, but it probably won't happen today.

fred3m · 2018-07-11T03:44:42Z

@TallJimbo I made substantial updates to the code based on your review, so would you mind looking over it again before I merge? The main changes are

Using multiple staticmethods as constructors to simplify the __init__ methods. The implementation might look a bit strange at first since, for example, MultibandImage.__init__ takes an image array instead of a list of single band Image objects, but because the array is the property that we want to make a view into, this is the logical property to pass to __init__.
Slight modification of the single band functions you added to slicing.py so that the multiband classes call the same functions, ensuring compatibility and less future maintenance.
Storing the MultibandMaskedImage in MultibandFootprint. I used the MultibandMaskedImage because HeavyFootprint doesn't (at this time) have a constructor that takes a one dimensional array of values. In the future it probably should, since building a HeavyFootprint from a 1D slice of a 2D array (filters, 1D values) would be beneficial, but since this ticket has already been dragging on for 2.5 months I feel like that should be left for a future ticket.

TallJimbo

I've taken another (much less thorough) look, and left another round of comments. Nothing big and structural this time, but there are a few smaller issues that should probably involve a sweep for similar occurrences throughout the changeset.

TallJimbo · 2018-07-16T18:33:45Z

doc/lsst.afw.multiband/multiband.rst

+
+    print(mImage["G"])
+    print(mImage[0])
+    print(mImage[:1].__repr__())


Better to write print(repr(mImage[:1])). Same throughout the docs.

TallJimbo · 2018-07-16T18:34:50Z

doc/lsst.afw.multiband/multiband.rst

+    #    333             if index < 0:
+    #
+    #IndexError: Indices must be <0 or between 1000 and 1199, received 1
+    #


I don't think the traceback itself is particularly useful here, and I'm sure it will get out-of-date fast (if it isn't already).

Sorry, I thought I made a note when I asked you to look over this again but it looks like I didn't. I updated everything except the docs, just in case there are any more API changes. I have a jupyter notebook that contains all of the statements in the docs and will re-run them all (to make sure that the docs are up to date) before merging.

TallJimbo · 2018-07-16T18:36:32Z

doc/lsst.afw.multiband/multiband.rst

+used to fill the single band objects (in fact the single band `Image` objects are initialized
+with pointers to the `Multiband.array`).
+Accessing this property may be necessary since images can not be set directly and must be
+updated by using the `array` property.


Why can't images be set directly? You have implemented MultibandImage.__setitem__, and setting pixel values via Image.__setitem__ should do what the caller expects.

TallJimbo · 2018-07-16T18:39:34Z

doc/lsst.afw.multiband/multiband.rst

+==================
+
+A `MultibandFootprint` is a collection of `HeavyFootprint` objects, one in each band,
+that do not necessarily need to have the same (or even overlapping) `SpanSet`s.


I believe this has changed.

TallJimbo · 2018-07-16T18:42:44Z

python/lsst/afw/multiband.py

+        ----------
+        offset: `Extent2I`
+            Amount to shift the bounding box in x and y.
+        """


Should document that this and the method above return new objects rather than operating in place.

Ah, I didn't realize that's what it was supposed to do, although looking back at the RFC now the name makes a lot more sense... I'll fix this for all of the classes.

Oh, I may have mispoken a bit between this comment and the one above. On the single-band objects, setXY0 does operate in-place (as should any method with that name). The names shiftedBy and shiftedTo (by being past tense) imply that they return new objects.

From the RFC, it looks like shiftedTo/By should return a view into the image, so a shallow copy?

Yes, shallow copy.

Arg, this is a breaking change until RFC-343 is implemented. Specifically, since the single band objects don't have shiftedBy and shiftedTo methods, the generic methods in MultibandBase don't know how to make a shallow copy of the singles and then modify only their bounding box while keeping a view to the original object.

So I'll leave the shiftedBy and shiftedTo methods in place (but with a NotImplementedError raised before the code executes, and also re-implement setXy0 with a note in DM-10781 to also fix the multiband classes.

TallJimbo · 2018-07-16T19:53:17Z

python/lsst/afw/image/multiband.py

+    kwargs: dict
+        Keyword arguments to pass to `_fullInitialize` to
+        initialize a new instance of an inherited class that are the
+        same in all bands.


I don't see any mention of _fullInitialize anywhere else in this file; is this documentation out of date?

Ah, yes, that was used before we changed the constructors.

TallJimbo · 2018-07-16T19:56:40Z

python/lsst/afw/image/multiband.py

+
+        see `imageFactory` for a description of parameters
+        """
+        return imageFactory(MultibandImage, filters, filterKwargs, singleType, **kwargs)


If you want this method to behave like the Factory method on one of the single-band classes, it'd need to support the same constructor arguments as the class's constructor. But I think that's probably unwise; Factory is an idiosyncratic crutch for a lack of a set of better-named factory methods (which has since largely been addressed since Factory was added to the single-band classes, but it's so broadly used now that it's hard to get rid of).

In any case, if you want this method to do something else, please give it another name. If you just wanted these classes to behave more like their single-band counterparts, don't worry about it in this particular case.

The main reason I wrote this was to give users the ability to pass a butler and create a MultibandExposure from a collection of keyword arguments, some of which may differ by filter. If we think that it's too much of a burden to support that I can remove both imageFactory and tripleFactory, otherwise I can just rename them something like makeImageFromKwargs.

I'm not opposed to keeping anything anything you have a use case for.

TallJimbo · 2018-07-16T20:04:46Z

python/lsst/afw/image/multiband.py

+    return cls(filters, image, mask, variance)
+
+
+def tripleFromArrays(cls, filters, image, mask, variance, bbox=None):


Verb-based name here, too.

TallJimbo · 2018-07-16T20:08:13Z

python/lsst/afw/image/multiband.py

+        ----------
+        coords: `Point2D` or `tuple`
+            Coordinates to evaluate the PSF. If `coords` is `None`
+            then `Psf.getAveragePosition()` is used.


We tend to use "coord" to mean sky coordinates; it'd be a bit more in keeping withe precedent and a bit less confusing to stack users to call this "position".

TallJimbo · 2018-07-16T20:16:30Z

python/lsst/afw/image/slicing.py

@@ -87,67 +87,40 @@ def handleNegativeIndex(index, size, origin, default):
    return index


-def makePointFromIndices(x, y, origin, bbox):
-    """Create a Point2I from an x, y pair, correctly handling negative indices.
+def makeBoxFromSlices(sliceArgs, bboxGetter):


Since this now does what interpretSliceArgs used to do (I think?) could we rename it to interpretSliceArgs and come up with a new name for the new thing? Or do we not need the thing called interpretSliceArgs anymore, since I don't see it used by __getitem__ and __setitem__ below?

In any case, it definitely shouldn't be called somethign Box- or Slice-specific, since it also works with single-pixel indexing arguments.

I think that we need two different functions, but I'm open to changing the names.

Because most of the multiband classes slice arrays, interpretSliceArgs returns indices or slices, which is what the multiband classes need. However single band objects require the additional step of transforming those indices into either a Box2I or Point2I, so the new name was supposed to convey that this is used for stack primitives.

Do you have a suggestion for a better name?

How about translateSliceArgs?

TallJimbo reviewed May 11, 2018

View reviewed changes

fred3m force-pushed the tickets/DM-13911 branch from 4cab711 to 415db89 Compare June 5, 2018 15:20

fred3m force-pushed the tickets/DM-13911 branch from 4973989 to 9623fc7 Compare June 22, 2018 19:41

TallJimbo requested changes Jun 29, 2018

View reviewed changes

TallJimbo approved these changes Jul 6, 2018

View reviewed changes

fred3m force-pushed the tickets/DM-13911 branch from aaf0d1d to 199440c Compare July 11, 2018 03:31

TallJimbo approved these changes Jul 16, 2018

View reviewed changes

fred3m force-pushed the tickets/DM-13911 branch 2 times, most recently from f71f2b5 to 6d6a42b Compare July 19, 2018 17:21

fred3m added 19 commits July 19, 2018 14:08

Create MultibandBase class

51429b4

Create multiband image classes

cf229de

Add setPeakCatalog method to Footprint

1883aab

Create MultibandFootprint

456f53a

Create tests for multiband classes

891ff50

Create docs for multiband classes

dfd94f8

Add newline

83cd46c

Move __init__ docs to class docstring

f950335

Make copy and _slice abstractmethods

ac4c3bd

Fixes based on reviewer feedback

7e3f6ac

Do not use int to index filter dimension

e8f6a14

Replace setBBox with shiftedBy and shiftedTo

14e34d9

More reviewer feedback fixes

6c521f4

Fix MultibandBask based on review commments

5552fa4

Return a tuple when selecting single pixel from MultibandMaskedImage

32c55b0

Do not cache MultibandExposure PsfImage

4c1105e

Use both computeKernelImage and computeImage

597481b

Update tests based on reviewer feedback

d128d5c

More small reviewer changes

56c3a6d

fred3m added 9 commits July 19, 2018 14:08

Use different constructors for MultibandImage and MultibandMask

b8d8c81

Use multiple constructors for MultibandMaskedImage and MultibandExposure

2e50b9e

Remove singleType from Multiband classes

8dbdd31

Fix flake8 errors

54588d4

Create new constructors

0f2b16b

Update single band slicing and use for multiband image slicing

2e4580a

update based on review comments

24b825f

Fix typo in MultibandFootprint

c98579d

Fixed circular dependency

aa2c125

fred3m force-pushed the tickets/DM-13911 branch from 6d6a42b to aa2c125 Compare July 19, 2018 21:40

Fix flake8 errors

8033fe8

fred3m merged commit 7a076d4 into master Jul 20, 2018

ktlim deleted the tickets/DM-13911 branch August 25, 2018 06:44

		from lsst.afw.image.multiband import MaskedPixel, MultibandMaskedPixel, MultibandExposure


		def _testImageFilterSlicing(cls, mImage, singleType, bbox, value):

		return cls(filters, image, mask, variance)


		def tripleFromArrays(cls, filters, image, mask, variance, bbox=None):

tickets/DM-13911: Create multiband classes #353

tickets/DM-13911: Create multiband classes #353

Conversation

fred3m commented May 10, 2018

TallJimbo May 11, 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

TallJimbo 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

TallJimbo commented Jul 6, 2018

fred3m commented Jul 11, 2018

TallJimbo 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

TallJimbo May 11, 2018 •

edited