DM-32402: Add TractBuilders and sub-patch cell capability. #56

erykoff · 2021-10-28T22:09:37Z

No description provided.

TallJimbo · 2021-11-02T14:10:13Z

python/lsst/skymap/cellInfo.py

+        """
+        return self._sequentialIndex
+
+    sequentialIndex = property(getSequentialIndex, None)


I have not yet looked at the PR in detail, but I wanted to make one point up-front based on what I knew was coming: I think we should make all of these new properties snake_case. They're brand new, so there's no breakage from doing so, and there are no existing camelCase properties and relatively few camelCase public attributes, so the internal-to-this-package inconsistency wouldn't actually be too. The new cell_coadds package that will make the most use of these will be snake_case throughout, so it'll be good to have more consistency with that, and of course it'd be much harder to change these anytime later.

One more side note: I think property(getSequentialIndex) should work too, and seems a bit better than the explicit None.

I was wondering about this, and now it is done.

TallJimbo

I've left a few minor comments. I haven't looked super closely at everything - I gather a lot is copied from existing code in the package, and I saw some things move around, and I know Arun is reviewing, too.

TallJimbo · 2021-11-03T02:59:20Z

python/lsst/skymap/patchInfo.py

    """

-    def __init__(self, index, innerBBox, outerBBox):
+    def __init__(self, index, innerBBox, outerBBox,
+                 cellInnerDimensions=Extent2I(0, 0), cellBorder=0,


Extent2I is actually mutable, so it shouldn't be used as a default argument (maybe someday I'll get around to DM-21148 and fix that, but don't hold your breath).

TallJimbo · 2021-11-03T03:01:52Z

python/lsst/skymap/patchInfo.py

+        self._cellInnerDimensions = cellInnerDimensions
+        self._cellBorder = cellBorder
+        if numCellsPerPatchInner == 0:
+            self._numCells = Extent2I(0, 0)


Using an Extent2I for a number of cells rather than something with dimensions of pixels doesn't quite feel right; if it's not a big inconvenience for arithmetic, it might be better to use a tuple or separate x, y attributes here (or that new named tuple).

The numPatches was always Extent2I (written by Russell Owen 10 years ago) it just wasn't documented. But as far as I can tell it was only used by anybody getting this property as a tuple except for one of the tests. So I'll make it the Index2D named tuple.

TallJimbo · 2021-11-03T03:04:09Z

python/lsst/skymap/baseSkyMap.py

-        dtype=int,
-        default=100,
+    tractBuilder = tractBuilderRegistry.makeField(
+        "Tract building algorithm",


Suggested change

"Tract building algorithm",

"Algorithm for creating the patches within a tract.",

TallJimbo · 2021-11-03T03:04:26Z

python/lsst/skymap/cellInfo.py

+from .detail import makeSkyPolygonFromBBox
+
+
+class CellInfo():


Suggested change

class CellInfo():

class CellInfo:

TallJimbo · 2021-11-03T03:08:38Z

python/lsst/skymap/patchInfo.py

+
+        nx, ny = self.getNumCells()
+        x = sequentialIndex % nx
+        y = (sequentialIndex - x) // nx


I would have written this as just

y = sequentialIndex // nx

I'm not sure that's better, but they are the same, right, because x < nx?

I'm just copying what was there before! But you are correct.

TallJimbo · 2021-11-03T03:09:13Z

python/lsst/skymap/tractBuilder.py

+from .detail import Index2D
+
+__all__ = ["tractBuilderRegistry", "BaseTractBuilder",
+           "LegacyTractBuilder", "CellTractBuilder"]


__all__ should go above all of the import statements.

TallJimbo · 2021-11-03T03:09:44Z

python/lsst/skymap/tractBuilder.py

+
+
+class BaseTractBuilder(metaclass=abc.ABCMeta):
+    """Base class for tract builders.


Suggested change

"""Base class for tract builders.

"""Base class for algorithms that define the patches within a tract.

TallJimbo · 2021-11-03T03:12:36Z

python/lsst/skymap/tractBuilder.py

+        # This should probably be the same as above because we only
+        # care about the INNER dimensions.
+        patchInd = tuple(int(pixelInd[i]/self._patchInnerDimensions[i]) for i in range(2))
+        return self.getPatchInfo(patchInd, wcs)


Do you expect derived classes to ever need to override findPatch and/or findPatchList? If not, maybe they should go back in TractInfo instead.

TallJimbo · 2021-11-03T03:13:22Z

python/lsst/skymap/tractBuilder.py

+                     for yInd in range(llPatchInd[1], urPatchInd[1]+1))
+
+    def getPatchBorder(self):
+        return self._patchBorder


Is this relevant for all tractBuilders?

At the moment, yes. Both legacy and cells have a border region, it's just one is an arbitrary number of pixels, the other is an arbitrary number of cells.

Hmm. If it means different things for different builders, I would be inclined to get rid of it if we possibly can.

It is always pixels that are returned here. It's just how it's specified in the config. You can specify the number of pixels directly (legacy) or the number of cells (cells), and since each cell has an explicit size you know how big the border is.

Ok, that's not too bad. Still, if nothing calls it, I'd vote for dropping it.

It's used in how to define the patches that make the tract. Dropping it would require some other big changes.

Ok, I'd hoped because the patches have different geometry on the edges, it might not have been needed, but if it is, fine.

arunkannawadi

I took a more detailed approach, mainly focus on what I thought I would need for my code. A number of suggestions, but all extremely minor.

arunkannawadi · 2021-11-05T16:24:09Z

python/lsst/skymap/detail/utils.py

+    return lsst.sphgeom.ConvexPolygon([sp.getVector() for sp in skyPoints])
+
+
+class Index2D(NamedTuple):


This needs a docstring, mainly to provide context on its use cases.

arunkannawadi · 2021-11-05T16:39:04Z

python/lsst/skymap/patchInfo.py

+        IndexError
+            If index is out of range.
+        """
+        if self._numCells[0] == 0 or self._numCells[1] == 0:


Given that _numCells are named tuples, it's best to address the components with x and y.

arunkannawadi · 2021-11-05T16:40:25Z

python/lsst/skymap/patchInfo.py

+                _index = self.getCellIndexPair(index)
+            else:
+                _index = Index2D(*index)
+        if (not 0 <= _index.x < self._numCells[0]) \


Suggested change

if (not 0 <= _index.x < self._numCells[0]) \

if (not 0 <= _index.x < self._numCells.x) \

arunkannawadi · 2021-11-05T16:40:37Z

python/lsst/skymap/patchInfo.py

+            else:
+                _index = Index2D(*index)
+        if (not 0 <= _index.x < self._numCells[0]) \
+                or (not 0 <= _index.y < self._numCells[1]):


Suggested change

or (not 0 <= _index.y < self._numCells[1]):

or (not 0 <= _index.y < self._numCells.y):

arunkannawadi · 2021-11-05T17:11:42Z

python/lsst/skymap/tractBuilder.py

+    )
+
+    def validate(self):
+        if len(self.patchInnerDimensions) != 2:


This is already enforced by setting length=2 in patchInnerDimensions and is therefore redundant.

In my testing, that did not appear to be true.

Ah, ListField's documentation was somewhat confusing. Calling config.validate() will raise an Exception if length is not two without explicitly defining it here.
https://github.com/lsst/pex_config/blob/ae7742993c3fa92bf5ac1d9ab0c298c95e81930a/python/lsst/pex/config/listField.py#L343..L345

Huh. I thought I tested this and it didn't work as I expected, but you're right. So I've deleted that redundant check. Thanks!

arunkannawadi · 2021-11-05T17:56:03Z

python/lsst/skymap/patchInfo.py

        x,y index of patch (a pair of ints)
    innerBBox : `lsst.geom.Box2I`
        inner bounding box
    outerBBox : `lsst.geom.Box2I`
        inner bounding box
+    sequentialIndex : `int`
+        Patch sequential index
+    tractWcs : `lsst.afw.geom.SkyWcs`


Suggested change

tractWcs : `lsst.afw.geom.SkyWcs`

tractWcs : `lsst.afw.geom.SkyWcs`, optional

arunkannawadi · 2021-11-05T17:58:05Z

python/lsst/skymap/patchInfo.py

        self._innerBBox = innerBBox
        self._outerBBox = outerBBox
+        self._wcs = tractWcs


Either tractWCS should not be optional (not ideal) or self._wcs should take the tract's WCS.

I'm not sure what the comment is here?

Let me try to rephrase my comment.

Since PatchInfo can be instantiated with tractWCS set to None (default), self._wcs will be set to None. I don't see a way where _wcs can be set later after instantiation. This would cause callinggetInnerSkyPolygon and getOuterSkyPolygon with tractWCS=None fail (default again). So we should not allow a PatchInfo object to be constructed without a tractWCS.

I recollect discussing about overriding the WCS attached to the object when calling those methods, but don't recollect discussing about needing to create PatchInfo without a WCS. If you see a compelling need, then please add to the docstring that if initialized without a WCS, they must necessarily be passed in methods such as getInnerSkyPolygon.

Oh, I see your point. I think that there's no way for anything legitimate (not monkey-patched or otherwise hacked) to get a PatchInfo without a wcs. So I could make this required at instantiation, or at least Raise if it doesn't have a valid wcs. Which would you prefer?

I would prefer making it required instead of raising an exception. I could imagine trying to create a PatchInfo directly for testing/playing around and would like to know that I need to provide a WCS to begin with.

arunkannawadi · 2021-11-05T18:00:44Z

python/lsst/skymap/patchInfo.py

+        else:
+            if not isinstance(index, Iterable):
+                raise ValueError("Input index is not an iterable.")
+            if len(index) != 2:


The Index2D constructor (or more accurately the NamedTuple constructor) takes care of this internally that I don't think this check needs to be in here.

arunkannawadi · 2021-11-05T18:07:12Z

python/lsst/skymap/tractBuilder.py

+
+    def getPatchInfo(self, index, tractWcs):
+        # This should always be initialized
+        assert self._initialized


Should this raise an AssertionError wihout any helpful message, or should we explicitly raise an exception? I understand assert statement are removed from the production builds, but during the development stage I don't think this is going to help and shouldn't be a significant overhead in production either.

arunkannawadi · 2021-11-05T18:07:53Z

python/lsst/skymap/tractBuilder.py

+
+    def getPatchInfo(self, index, tractWcs):
+        # This should always be initialized
+        assert self._initialized


Similar comment about asserting as an earlier comment.

erykoff added 2 commits October 28, 2021 15:08

Add TractBuilders and sub-patch cell capability.

0d84da1

Add configuration for number of cells in the patch border.

738047e

erykoff force-pushed the tickets/DM-32402 branch from c80cdd5 to 3cdb16a Compare November 1, 2021 15:06

TallJimbo reviewed Nov 2, 2021

View reviewed changes

Add properties, patch and cell wcs access, and tests.

cf96cfa

erykoff force-pushed the tickets/DM-32402 branch from 3cdb16a to cf96cfa Compare November 2, 2021 15:20

erykoff added 2 commits November 2, 2021 08:59

Add check if coordinate has nan values.

5bf378b

Make patch and cell indexes a named tuple.

bc8a3f0

TallJimbo approved these changes Nov 3, 2021

View reviewed changes

arunkannawadi approved these changes Nov 5, 2021

View reviewed changes

erykoff force-pushed the tickets/DM-32402 branch 2 times, most recently from 38a79f5 to eb4b260 Compare November 5, 2021 21:52

Refactor locations of methods; return Index2D for numPatches/Cells

1ead253

erykoff force-pushed the tickets/DM-32402 branch from eb4b260 to 1ead253 Compare November 8, 2021 16:50

erykoff merged commit 50866e6 into master Nov 8, 2021

erykoff deleted the tickets/DM-32402 branch November 8, 2021 23:30

arunkannawadi mentioned this pull request Apr 12, 2022

What stack version to read NERSC Gen3 skymap? LSSTDESC/desc-help#83

Closed

10 tasks

	"Tract building algorithm",
	"Algorithm for creating the patches within a tract.",



		class BaseTractBuilder(metaclass=abc.ABCMeta):
		"""Base class for tract builders.

	"""Base class for tract builders.
	"""Base class for algorithms that define the patches within a tract.

		return lsst.sphgeom.ConvexPolygon([sp.getVector() for sp in skyPoints])


		class Index2D(NamedTuple):

	if (not 0 <= _index.x < self._numCells[0]) \
	if (not 0 <= _index.x < self._numCells.x) \

	or (not 0 <= _index.y < self._numCells[1]):
	or (not 0 <= _index.y < self._numCells.y):

	tractWcs : `lsst.afw.geom.SkyWcs`
	tractWcs : `lsst.afw.geom.SkyWcs`, optional

DM-32402: Add TractBuilders and sub-patch cell capability. #56

DM-32402: Add TractBuilders and sub-patch cell capability. #56

Conversation

erykoff commented Oct 28, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi 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