DM-17451: raise when columnviews access undefined keys #431

jdswinbank · 2019-01-25T18:34:25Z

No description provided.

parejkoj

A handful of questions and comments.

Two broader points:

I would rebase squash the tests with the new C++ code, since they clearly go together.
It may be out of scope, but I feel like testColumnView and _testFluxSlot have too much in them right now: it's hard to keep track of what is going on and what should or shouldn't be available at any given point. It may be too annoying given all the other code to manage the catalogs, but I wonder if you couldn't put them in their own test case, so that the success/failure condition is more obvious?

parejkoj · 2019-01-29T06:06:53Z

src/table/BaseColumnView.cc

+    if (!key.isValid()) {
+        throw LSST_EXCEPT(
+            pex::exceptions::LogicError,
+            "Key is not valid (if this is a SourceCatalog, make sure slot aliases have been set up).");


Why SourceCatalog here? Can slots not be set on SimpleCatalogs? What other types of catalogs can or cannot have slots?

Slots are for SourceTables (or Catalogs) only (see e.g. https://github.com/lsst/afw/blob/master/include/lsst/afw/table/slots.h#L24).

parejkoj · 2019-01-29T06:08:03Z

tests/test_simpleTable.py

+        for keyType in ["Angle", "ArrayB", "ArrayD", "ArrayF", "ArrayI",
+                        "ArrayU", "B", "D", "F", "Flag", "I", "L", "U"]:
+            # Default-constructed key is invalid
+            invalidKey = getattr(lsst.afw.table, f"Key{keyType}")()


Neat use of format strings. I need to use those more.

parejkoj · 2019-01-29T06:08:51Z

tests/test_simpleTable.py

+            # Default-constructed key is invalid
+            invalidKey = getattr(lsst.afw.table, f"Key{keyType}")()
+            self.assertFalse(invalidKey.isValid())
+            self.assertRaises(lsst.pex.exceptions.LogicError, catalog.get, invalidKey)


I find the context manager version easier to understand the intent:

with self.assertRaises(LogicError): catalog.get(invalidKey)

I think this is just taste. Personally, I prefer the non-context-manager version when testing the result of a simple function call; the context manager is obviously better when dealing with more involved logic.

The testing guide prefers the context manager. Currently about 90% of the LSST code base complies with that statement (and I definitely prefer the context manage for code readability).

https://developer.lsst.io/python/testing.html?highlight=assertraises

parejkoj · 2019-01-29T06:09:18Z

src/table/BaseColumnView.cc

@@ -69,6 +69,11 @@ std::shared_ptr<BaseTable> BaseColumnView::getTable() const { return _impl->tabl

 template <typename T>
 typename ndarray::ArrayRef<T, 1> const BaseColumnView::operator[](Key<T> const &key) const {
+    if (!key.isValid()) {
+        throw LSST_EXCEPT(
+            pex::exceptions::LogicError,


Should this raise LogicError or KeyError?

I reckon LogicError, for consistency with the behaviour when accessing an invalid key on a single record. I was actually surprised that was a LogicError, but I think changing it on this ticket is out of scope.

parejkoj · 2019-01-29T06:11:58Z

tests/test_sourceTable.py

@@ -639,7 +643,8 @@ def _testFluxSlot(self, slotName):
                                 type=np.float64, doc="flux uncertainty")
        flagKey = schema.addField("%s_flag" % (baseName,),
                                  type="Flag", doc="flux flag")
-        table = lsst.afw.table.SourceTable.make(schema)
+        catalog = lsst.afw.table.SourceCatalog(schema)
+        table = catalog.table


I wish someone would explain to me the difference between an Table and a Catalog...

Roughy speaking (and I'm sure Jim or somebody else who knows better will be along to correct me in a moment):

Tables are (confusingly named) record factories.

Catalogs are containers of records.

In this particular case, we were previously only testing access to slots on individual records, so we only needed a table (a factory to create those records). I extended the test to test that the slot worked properly on a catalog (ie, a collection of records) too; obviously, that means we need to create the catalog for testing.

parejkoj · 2019-01-29T06:13:04Z

tests/test_sourceTable.py

+        self.catalog.table.schema.getAliasMap().erase("slot_Centroid")
+        self.catalog.table.schema.getAliasMap().erase("slot_Shape")
+        for quantity in ["X", "Y", "Ixx", "Iyy", "Ixy"]:
+            self.assertRaises(lsst.pex.exceptions.LogicError, getattr(self.catalog, f"get{quantity}"))


Again with the with. Please add a comment above this explaining what this test is doing: it took me a bit to figure it out.

parejkoj · 2019-01-29T06:17:14Z

tests/test_sourceTable.py

        # And we should be able to delete the slot, breaking the mapping.
        table.schema.getAliasMap().erase("slot_%s" % (slotName,))
        self.assertFalse(getattr(table, "get%sSlot" % (slotName,))().isValid())
        self.assertNotEqual(getattr(table, "get%sSlot" % (slotName,))().getMeasKey(), instFluxKey)
        self.assertNotEqual(getattr(table, "get%sSlot" % (slotName,))().getErrKey(), errKey)
        self.assertNotEqual(getattr(table, "get%sSlot" % (slotName,))().getFlagKey(), flagKey)

+        with self.assertRaises(lsst.pex.exceptions.LogicError):
+            getattr(catalog, f"get{instFluxName}")()
+        with self.assertRaises(lsst.pex.exceptions.LogicError):


Why should these two raise? I would have thought they would work?

These are referring to keys which are defined as part of the slot. When the slot is erased (at line 675, above), all the keys defined as part of it are invalidated. Accessing via those invalid keys raises.

I'll add a comment to that effect.

jdswinbank force-pushed the tickets/DM-17451 branch from c1d1b4a to 8fa4541 Compare January 25, 2019 18:35

parejkoj reviewed Jan 29, 2019

View reviewed changes

jdswinbank force-pushed the tickets/DM-17451 branch from 8fa4541 to 8e3445a Compare January 29, 2019 07:02

jdswinbank added 4 commits January 29, 2019 09:38

Test for raise on using an invalid key with ColumnView.

8a89a5e

Check that column slot accessors fail appropriately.

d26e36a

Check for valid keys when accessing table columns.

ad25dd5

Fix typo.

0f740b8

jdswinbank force-pushed the tickets/DM-17451 branch from 8e3445a to 0f740b8 Compare January 29, 2019 17:45

jdswinbank merged commit 0f740b8 into master Feb 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-17451: raise when columnviews access undefined keys #431

DM-17451: raise when columnviews access undefined keys #431

jdswinbank commented Jan 25, 2019

parejkoj left a comment

parejkoj Jan 29, 2019

jdswinbank Jan 29, 2019

parejkoj Jan 29, 2019

parejkoj Jan 29, 2019

jdswinbank Jan 29, 2019

timj Jan 29, 2019 •

edited

parejkoj Jan 29, 2019

jdswinbank Jan 29, 2019

parejkoj Jan 29, 2019

jdswinbank Jan 29, 2019 •

edited

parejkoj Jan 29, 2019

parejkoj Jan 29, 2019

jdswinbank Jan 29, 2019

DM-17451: raise when columnviews access undefined keys #431

DM-17451: raise when columnviews access undefined keys #431

Conversation

jdswinbank commented Jan 25, 2019

parejkoj 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

timj Jan 29, 2019 • 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

jdswinbank Jan 29, 2019 • 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

timj Jan 29, 2019 •

edited

jdswinbank Jan 29, 2019 •

edited