DM-14428: Spatial joins tables and views #55

TallJimbo · 2018-06-19T14:29:16Z

No description provided.

andy-slac

@TallJimbo, I'm not completely done reviewing yet, but have some comments you may want to start looking at while I'm finishing my part.

andy-slac · 2018-06-19T23:14:16Z

python/lsst/daf/butler/core/registry.py

@@ -69,7 +69,8 @@ def setConfigRoot(cls, root, config, full):
            from defaults when Butler instances are constructed
            should be copied from `full` to `Config`.
        """
-        raise NotImplementedError()
+        for key in ("registry.skypix.name", "registry.skypix.level"):


Should it be registry.skypix.cls instead of registry.skypix.name?

andy-slac · 2018-06-19T23:18:10Z

config/schema.yaml

+            type: int
+            primary_key: true
+            nullable: false
+            doc: visit


andy-slac · 2018-06-19T23:23:11Z

config/schema.yaml

+            doc: visit
+          -
+            name: region
+            type: string


type is string in this table but blob in Tract/Patch tables. Should we instead introduce special region column type and use type magic name rather than column magic name in Python code?

I've made it a blob here too. Is there a way to use magic Python conversions in a way that is truly RDBMS-independent? (Note I'd like to avoid even exposing that we're using SQLAlchemy publicly).

I don't think we need magic here, we are reading this schema ourselves and transform it into backed-specific schema so we have complete freedom. My worry is that using low-level (physical) type here instead of logical (region) we limit ourselves to a particular choice of storage type. I can imagine for some backends BLOB may be too inefficient and you may want to store regions (which should be relatively short strings of bytes) in more efficient storage type.

Ah, I just misunderstood what you were suggesting. I'll go ahead and make that change (while still mapping region columns to sqlalchemy.LargeBinary for now).

andy-slac · 2018-06-19T23:25:25Z

config/schema.yaml

+              this Visit+Sensor combination.  This is expected to be more
+              precise than the SkyPixels associated with the Visit+Sensor (see
+              VisitSensorSkyPixJoin), but may still be larger than the true
+              region as long as it fully covers it.


Do we describe string representation for regions anywhere or do we depend on sphgeom for it? If latter them maybe add couple of words here.

We do rely on sphgeom for this. I'll add some docs.

andy-slac · 2018-06-19T23:28:45Z

config/schema.yaml

+          foreignKeys:
+          -
+            src: camera
+            tgt: Camera.camera


I'm not sure this foreign key is useful at all.

It is indeed redundant with other foreign key constraints. I'll remove it.

andy-slac · 2018-06-20T00:05:29Z

python/lsst/daf/butler/core/dataUnit.py

@@ -270,7 +312,7 @@ def _initDataUnitNames(self, config):
        Parameters
        ----------
        config : `SchemaConfig`
-            Schema configuration describing `DataUnit` relations.
+            The `DataUnit` component of a `SchemaConfig`.


DataUnits (or dataUnits)?

andy-slac · 2018-06-20T00:05:45Z

python/lsst/daf/butler/core/dataUnit.py

-        config : `SchemaConfig`
-            Schema configuration describing `DataUnit` relations.
+        config : `Config`
+            The `DataUnit` component of a `SchemaConfig`.


andy-slac · 2018-06-20T00:08:05Z

python/lsst/daf/butler/core/dataUnit.py

            if 'tables' in dataUnitJoinDescription and builder is not None:
                for tableName, tableDescription in dataUnitJoinDescription['tables'].items():
                    table = builder.addTable(tableName, tableDescription)
+                    isView = "sql" in tableDescription


Could "sql" in YAML potentially mean something else than view?

In general, yes (see ExposureRangeJoin), inside a tables section it always means a view.

andy-slac · 2018-06-20T00:13:37Z

python/lsst/daf/butler/core/registry.py

@@ -115,5 +116,7 @@ def fromConfig(registryConfig, schemaConfig=None, create=False):
    def __init__(self, registryConfig, schemaConfig=None, create=False):
        assert isinstance(registryConfig, RegistryConfig)
        self.config = registryConfig
+        pixelizationCls = doImport(self.config["skypix.cls"])


Make self.pixelization a property so that import is deferred until it's actually needed?

andy-slac · 2018-06-20T00:24:41Z

python/lsst/daf/butler/registries/sqlRegistry.py

@@ -788,6 +788,12 @@ def addDataUnitEntry(self, dataUnitName, values):
        values : `dict`
            Dictionary of ``columnName, columnValue`` pairs.

+        If ``values`` includes a "region" key, `setDataUnitRegion` will
+        automatically be called to set it any any associated spatial join


andy-slac

I'm done with review, it looks OK but I think there may be couple of thing that need change, please see my comments.

andy-slac · 2018-06-20T16:26:07Z

config/schema.yaml

@@ -769,7 +790,8 @@ schema:
      lhs:
        - Visit
        - Sensor
-      rhs: SkyPix
+      rhs:
+        - SkyPix


For simple cases like this could we possibly use more compact inline style?

lhs: [Visit, Sensor] rhs: [SkyPix]

Sure (I don't have a preference - it's a question of whether compactness or consistency is more valuable).

andy-slac · 2018-06-20T16:52:05Z

python/lsst/daf/butler/formatters/fitsExposureFormatter.py

+        fileDescriptor : `FileDescriptor`
+            Identifies the file to read, type to read it into and parameters
+            to be used for reading.
+        comp : `str`, optional


Can you spell this parameter name completely? comp is such a common prefix, hard to tell if it means compare/computer/compost/compassion 😉
Also, base class read() method does not seem to have this parameter, I can't figure out how it can be used.

This looks like it's my fault because I call it comp in fileFormatter. posixDatastore does use that form of formatter.read(). You are right I forgot to change the abstract base class. The change here is to allow Exposure components to be read without requiring the entire Exposure to be read.

I've renamed this to component and added it to the Formatter base class.

andy-slac · 2018-06-20T17:00:40Z

python/lsst/daf/butler/formatters/fitsExposureFormatter.py

+                # Just pass parameters into kwargs for constructor, but check that we recognize them.
+                kwds.update(fileDescriptor.parameters)
+                if not self.parameters.issuperset(kwds.keys()):
+                    raise KeyError("Unrecognized parameter key(s): {}".format(self.parameters - kwds.keys()))


Should it be kwds.keys() - self.parameters instead?

andy-slac · 2018-06-20T17:21:21Z

python/lsst/daf/butler/registries/sqlRegistry.py

+        # inserted previously and this is an improved region for them, or
+        # because the region is associated with a single DataUnit instance and
+        # is hence part of that DataUnit's main table.
+        if not new or (len(dataUnitNames) == 1 and table == dataUnit.table):


I guess dataUnit here originates from the loop above? Not illegal but potentially fragile.

Right; it looks a bit weird to my C++-trained eyes, too, but I think this is an example of exactly why Python allows loop variables to live beyond their loops.

timj · 2018-06-20T17:46:19Z

python/lsst/daf/butler/formatters/fitsExposureFormatter.py

-import os.path
-
-from lsst.daf.butler.formatters.fileFormatter import FileFormatter
+from ..core.formatter import Formatter


I was deliberately using the public formatter interface rather than doing a relative import, since this is what external formatters would look like and it seemed wrong to use internals for them.

timj · 2018-06-20T17:52:18Z

ups/daf_butler.table

@@ -9,6 +9,7 @@ setupRequired(utils)
 setupRequired(daf_persistence)
 setupRequired(afw)

+envPrepend(PATH, ${PRODUCT_DIR}/bin)
 envPrepend(LD_LIBRARY_PATH, ${PRODUCT_DIR}/lib)


whereby I notice that we are setting library search paths when we should not be since this is pure python package.

timj · 2018-06-20T17:53:07Z

python/lsst/daf/butler/registries/sqliteRegistry.py

@@ -65,6 +65,7 @@ def setConfigRoot(cls, root, config, full):
            from defaults when Butler instances are constructed
            should be copied from `full` to `Config`.
        """
+        super(SqliteRegistry, cls).setConfigRoot(root, config, full)


If super().setConfigRoot() is not going to work it might be worth having a comment here explaining why you are using the explicit form.

Ah, I thought super() wouldn't work in a classmethod, but it seemed I was wrong about that.

(e.g. VisitSensorRegion)

TallJimbo · 2018-06-26T19:46:14Z

@andy-slac, I believe I've addressed all comments. Please take another look and approve if you agree.

andy-slac · 2018-06-26T20:46:15Z

Approved, github says "andy-slac approved these changes 17 seconds from now" but I approved it 20 seconds ago 😄

andy-slac reviewed Jun 20, 2018

View reviewed changes

andy-slac requested changes Jun 20, 2018

View reviewed changes

timj reviewed Jun 20, 2018

View reviewed changes

TallJimbo added 18 commits June 26, 2018 13:22

Separate region part of DataUnit insertion.

1216dab

Add support for multi-DataUnit regions.

3f0ff2c

(e.g. VisitSensorRegion)

Include component Datasets in Gen2->Gen3 conversion.

f44d069

Slicing and more efficient component access for Exposure.

39fe0fe

Add metadata to Exposure StorageClass.

5f94268

Add observation regions to Gen3 conversion.

25b82ba

Include SkyPix definition in Registry configuration.

911dcfb

Use lists consistently in lhs, rhs for DataUnit joins.

4fecf0f

Improvements to DataUnitJoin support.

b645292

Populate SkyPix join tables when setting DataUnit regions.

21ed73d

Add PATH to EUPS environment variables.

158255e

Add view for AbstractFilter.

5406ba4

Improve docs for region columns in YAML.

d2af903

Use more concise YAML list from for DataUnitJoins.

0e704bf

Rename comp argument to component.

dd4d9fc

Remove unnecessary table file envvars.

0646e10

Use new-style super().

44c303f

Add custom column type for regions.

342b357

TallJimbo force-pushed the tickets/DM-14428 branch from 9da91bb to 342b357 Compare June 26, 2018 19:44

andy-slac approved these changes Jun 26, 2018

View reviewed changes

TallJimbo merged commit 05a3be9 into master Jun 28, 2018

ktlim deleted the tickets/DM-14428 branch August 25, 2018 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-14428: Spatial joins tables and views #55

DM-14428: Spatial joins tables and views #55

TallJimbo commented Jun 19, 2018

andy-slac left a comment

andy-slac Jun 19, 2018

andy-slac Jun 19, 2018

andy-slac Jun 19, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 26, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 19, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 19, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 20, 2018

andy-slac Jun 20, 2018

andy-slac Jun 20, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 20, 2018

andy-slac Jun 20, 2018

andy-slac left a comment

andy-slac Jun 20, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 20, 2018

timj Jun 20, 2018

TallJimbo Jun 26, 2018

andy-slac Jun 20, 2018

andy-slac Jun 20, 2018

TallJimbo Jun 26, 2018

timj Jun 20, 2018

timj Jun 20, 2018

timj Jun 20, 2018

TallJimbo Jun 26, 2018

TallJimbo commented Jun 26, 2018

andy-slac commented Jun 26, 2018

DM-14428: Spatial joins tables and views #55

DM-14428: Spatial joins tables and views #55

Conversation

TallJimbo commented Jun 19, 2018

andy-slac 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

andy-slac 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

TallJimbo commented Jun 26, 2018

andy-slac commented Jun 26, 2018