DM-13222: Add getScalar and getArray methods to PropertySet and PropertyList and prefer them #36

r-owen · 2018-06-06T20:30:02Z

No description provided.

timj · 2018-06-06T20:34:02Z

python/lsst/daf/base/propertyContainer/propertyContainerContinued.py

@@ -170,8 +205,7 @@ def setstate(self, state):
 class PropertySet:
    # Mapping of type to method names
    _typeMenu = {bool: "Bool",
-                 long: "LongLong",
-                 int: "Int",  # overwrites long on Python 3.x
+                 int: "Int",


Since all Python 3 ints are potentially very large, should this mapping be

int: "LongLong",

?

I was wondering the same thing. I just copied the old code, which over-wrote the long: "LongLong" entry in Python 3, leaving the int entry. But I was surprised the two entries were not swapped so that py3 retained the "LongLong" entry.

I'm wondering if @ktlim has an opinion about this. I'm certainly happy to change it.

This is not actually used at all now that there's _guessIntegerType().

So should I delete the entry or add a comment explaining that the exact integer type doesn't matter as long as it's some kind of integer, or...?

I'm pretty sure it isn't used at all, so it's best to delete it to make sure it isn't used accidentally.

Thank you. I deleted it with a code comment. The unit tests pass.

ktlim · 2018-06-11T21:57:01Z

python/lsst/daf/base/propertyContainer/propertyContainerContinued.py

-    """Extract a single Python value of unknown type"""
+    """Get a value of unknown type as a scalar or array
+
+    Return a scalar if there is only one value, else return an array


Also returns an array if asArray==True of course.

I see no evidence that the asArray argument is used by any code in our stack, except two unit tests in daf_base, so I am going to try removing it from PropertySet.get and PropertyList.get and see if Jenkins passes. If it does, this will make it much easier to implement shared base code for getArray and getScalar since we can change the argument to asScalar and only return a scalar if that is true.

DM-14770 provides an example of why the asArray argument may be good to delete. I found code that treats the second argument as a default value, in case the requested item is missing.

ktlim · 2018-06-11T22:00:33Z

python/lsst/daf/base/propertyContainer/propertyContainerContinued.py

+    if elemType:
+        return getattr(container, "getArray" + elemType)(name)[-1]
+
+    raise TypeError("Item {} is not numeric or string".format(name))


I think you could return a PropertyList, PropertySet, or PersistablePtr here; they're all scalar in this sense, and that would allow you to fully deprecate get().

But, failing that, since these two methods differ only in name and four characters ([-1]), wouldn't it be better to combine them? Or, even better, if get() is not to be removed, implement them in terms of get(..., asArray=True)?

Do you think that just getScalar should be able to return a PropertyList, PropertySet, or PersistablePtr, or that getArray should be able to do so as well?

I can see arguments for all 4 options, though I personally only like the first 3:

neither (the current code): PropertySet and the like are neither scalars nor arrays, so using get makes for cleaner and less surprising code. Also the user is less likely to be surprised by retrieving a PropertySet when they expected a number, string or list of either of those.

both: it's simple to return the obvious object if it's not numeric or string-like. It allows the user to iterate over all names and gets something reasonable back. The user just has to be careful with types, but that's implied by a container such as this anyway. Also this allows us to (in theory) deprecate get.

getScalar only: getArray always returns an array and PropertySet and the like are arguably more like scalars than arrays. Other than that, all the arguments to both also apply to this option. I think it would be acceptable, but I worry it might annoy and confuse users to have to remember which of getScalar and getArray can return PropertySet and which cannot.

getArray only: getArray returns the most "native" format -- since internally numeric and string data is stored in an array. But that's somewhat of an internal detail, so I consider this a very weak argument. This is by far my least favorite choice, since I think it would be difficult to explain why getArray can return PropertySet but getScalar cannot. I fear many users would be confused and frustrated.

After talking to @ktlim we agreed that getScalar will be able to return PropertySet and similar items, but that getArray will continue to raise. If I am successful in removing the asArray argument of get (without breaking existing code) then this will result in a fairly logical API. With the asArray argument it's a bit weird because get(name, asArray=True) can return PropertySet and getArray cannot.

As expressed in Slack, I'm OK with either "both" or "getScalar only".

ktlim · 2018-06-12T00:27:06Z

python/lsst/daf/base/propertyContainer/propertyContainerContinued.py

@@ -170,8 +205,7 @@ def setstate(self, state):
 class PropertySet:
    # Mapping of type to method names
    _typeMenu = {bool: "Bool",
-                 long: "LongLong",
-                 int: "Int",  # overwrites long on Python 3.x
+                 int: "Int",


This is not actually used at all now that there's _guessIntegerType().

timj · 2018-06-13T02:35:13Z

tests/test_PropertyList.py

+
+    And check that it raises a DeprecationWarning
+    """
+    with warnings.catch_warnings(record=True) as w:


Why not:

with self.assertWarns(DeprecationWarning): do_something()

The doc incorrectly stated that it always returned a scalar

to PropertySet and PropertyList. Expand existing unit tests to test them.

and PropertyList.get

using _propertyContainerGet with a new argument that controls whether the returned value is array, scalar or automatically chosen

of PropertySet and PropertyList, including getArray and getScalar

Remove python 2 support

fe749ca

r-owen force-pushed the tickets/DM-13222 branch from f2bb739 to efa4606 Compare June 6, 2018 20:31

timj reviewed Jun 6, 2018

View reviewed changes

ktlim approved these changes Jun 12, 2018

View reviewed changes

timj reviewed Jun 13, 2018

View reviewed changes

r-owen force-pushed the tickets/DM-13222 branch 5 times, most recently from 7b3d260 to 05867bb Compare June 13, 2018 16:52

r-owen added 10 commits June 13, 2018 09:52

Remove int from _typeMenu

a038498

git hide .coverage

a79fddd

Add support for Travis to run flake8

1196800

Fix doc error for _propertyContainerGet

fcc18e3

The doc incorrectly stated that it always returned a scalar

Modernize use of assertRaises

74bcb32

Add getArray and getScalar methods

5ae1199

to PropertySet and PropertyList. Expand existing unit tests to test them.

Remove unused asArray arg from PropertySet.get

7902e8f

and PropertyList.get

Re-implement getArray and getScalar

0852f85

using _propertyContainerGet with a new argument that controls whether the returned value is array, scalar or automatically chosen

Deprecate PropertySet.get and PropertyList.get

4dc2075

Add documentation for add, get and set methods

2b1e65c

of PropertySet and PropertyList, including getArray and getScalar

r-owen force-pushed the tickets/DM-13222 branch from 05867bb to 2b1e65c Compare June 13, 2018 16:53

r-owen merged commit 94bc79c into master Jun 13, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13222: Add getScalar and getArray methods to PropertySet and PropertyList and prefer them #36

DM-13222: Add getScalar and getArray methods to PropertySet and PropertyList and prefer them #36

r-owen commented Jun 6, 2018

timj Jun 6, 2018

r-owen Jun 6, 2018 •

edited

ktlim Jun 12, 2018

r-owen Jun 12, 2018

ktlim Jun 12, 2018

r-owen Jun 12, 2018

ktlim Jun 11, 2018

r-owen Jun 12, 2018

r-owen Jun 12, 2018 •

edited

ktlim Jun 11, 2018

r-owen Jun 12, 2018 •

edited

r-owen Jun 12, 2018

ktlim Jun 12, 2018

ktlim Jun 12, 2018

timj Jun 13, 2018

DM-13222: Add getScalar and getArray methods to PropertySet and PropertyList and prefer them #36

DM-13222: Add getScalar and getArray methods to PropertySet and PropertyList and prefer them #36

Conversation

r-owen commented Jun 6, 2018

Choose a reason for hiding this comment

r-owen Jun 6, 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

r-owen Jun 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Jun 12, 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

r-owen Jun 6, 2018 •

edited

r-owen Jun 12, 2018 •

edited

r-owen Jun 12, 2018 •

edited