Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-434

Add getScalar and getArray methods to PropertySet and PropertyList and prefer them

    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

            Hide
            rowen Russell Owen added a comment -

            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:

            • It is safer and more predictable than the current technique of getting array if > 1 value and a scaler if 1 value is neither.
            • It makes the Python more closely match the C++

            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.

            Show
            rowen Russell Owen added a comment - 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: It is safer and more predictable than the current technique of getting array if > 1 value and a scaler if 1 value is neither. It makes the Python more closely match the C++ 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.
            Hide
            tjenness Tim Jenness added a comment -

            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?

            Show
            tjenness Tim Jenness added a comment - 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?
            Hide
            rowen Russell Owen added a comment -

            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.

            Show
            rowen Russell Owen added a comment - 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.
            Hide
            ktl Kian-Tat Lim added a comment -

            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?

            Show
            ktl Kian-Tat Lim added a comment - 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?
            Hide
            rowen Russell Owen added a comment - - edited

            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.

            Show
            rowen Russell Owen added a comment - - edited 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.
            Hide
            rowen Russell Owen added a comment -

            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.

            Show
            rowen Russell Owen added a comment - 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.
            Hide
            ktl Kian-Tat Lim added a comment -

            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.

            Show
            ktl Kian-Tat Lim added a comment - 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.
            Hide
            rowen Russell Owen added a comment -

            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
            Show
            rowen Russell Owen added a comment - 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

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Watchers:
                John Swinbank, Kian-Tat Lim, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Planned End:

                  Summary Panel