Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
In Python PropertySet.get(name) will return a scalar if there is only one element, and an array if there is more than one element. Similarly for PropertyList. I think this encourages unsafe code. When expecting a scalar it is far too easy to expect there will only be one element and not check. Similarly, when expecting an array, one may be surprised to get a scalar if the array happens to only have one element.
I suggest we add two methods to PropertySet and PropertyList in Python:
- getScalar(name): return the last value of the array, like C++ get<T>
- getArray(name): return the data as an array, even if it only has one element. This is like C++ getArray<T>.
I further suggest that we deprecate get and use these new methods for all new code. It avoids obvious bugs and makes the intent clearer. Here are some before and after examples:
scalar = metadata.get("ascalar") # unsafe; breaks if more than one value
|
scalar = metadata.get("ascalar", asArray=True)[-1] # safe and clear but ugly
|
scalar = metadata.get("ascalar", True)[-1] # safe but unclear
|
scalar = metadata.getScalar("ascalar")
|
|
|
array = metadata.get("anarray") # unsafe; breaks if only one value
|
array = metadata.get("anarray", asArray=True) # safe and clear
|
array = metadata.get("anarray", True) # safe but unclear
|
array = metadata.getArray("anarray")
|
Note that the safe old style examples use the flag asArray. This is an optional argument to get that I didn't know existed until today. It is undocumented. I'm curious how many folks even knew about it, and are confident they know what it does.
Attachments
Issue Links
Activity
I'd like Kian-Tat Lim to comment on this. It looks good to me. When you say "deprecate" do you mean in the sense of "we'd rather you not use it" or in the sense that the python API will issue a warning?
I think a deprecation warning is only appropriate once existing code has been modified to use the new API. When we want to schedule that work is an open question.
I'm fine with this. One question I just thought of: is there any value to having the C++ get change to getScalar as well?
Kian-Tat Lim good point. It would improve consistency and clarity to rename the C++ method to getScalar as well, even though C++ requires an explicit template parameter (e.g. getScalar<int>). I'll give folks a bit more time to complain before adopting that change.
Note that I don't propose renaming getAsDouble and such. They have the same behavior in C++ and Python: return the final value, cast as requested.
After thinking about it more I am not keen to change get<type> to getScalar<type> in C+. The C+ API is already fine. It is true that it is a small improvement to consistency between the C++ and Python, but they don't match either way and I don't think it is worth the effort. It could also be done at a later point if desired.
After thinking about it more I am not keen to change get<type> to getScalar<type> in C++.
OK with me; it was just a thought.
Adopted as follows:
- Add PropertySet.getScalar(name) which returns the final value, like get<type> in C++
- Document that get is deprecated in favor of getScalar and the existing getArray
- Once code has been updated to use getScalar add a deprecation warning to get
Other than a few comments in Slack I've seen no discussion. Robert Lupton said something about FITS that I can't find right now.
I did not not intended this RFC to be about FITS. I proposed it for two main reasons:
One issue is what getScalar should do if there is more than one value. This RFC proposes to silently return the last value, just like C++ get<type> ("most recent wins"). Another option would be to raise an exception. I prefer the former but am open to the latter, or having a doRaise argument, if others feel strongly enough about it.