Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-11052

Enhance afw table to support variable-length string data

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      6
    • Epic Link:
    • Sprint:
      Alert Production S17 - 6, Alert Production F17 - 7
    • Team:
      Alert Production

      Description

      In order to persist lsst.afw.geom.SkyWcs in afw tables and FITS binary tables we want to expand afw table to support variable-length strings. It already supports variable-length numeric arrays, so Jim Bosch thinks adding this feature should be fairly straightforward.

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-9679 [ 30784 ]
            rowen Russell Owen made changes -
            Link This issue blocks DM-10765 [ DM-10765 ]
            rowen Russell Owen made changes -
            Priority Undefined [ 10000 ] Major [ 3 ]
            Hide
            rowen Russell Owen added a comment -

            If you have time I would appreciate a full review. If not, I would still appreciate your weighing in on two things:

            • How can I test my changes in FieldBase.h to FieldBase<std::string>::get[Const]Reference in Python? I think the changes are necessary if this code is reachable at all, but all of my attempts to access a string field using reference notation failed.
            • Is my change to error handling in BaseRecord.cc commit 0c65e9f acceptable? I think the old test was inadequate because it ignored the case that the left-hand side was fixed and the right-hand side variable.
            Show
            rowen Russell Owen added a comment - If you have time I would appreciate a full review. If not, I would still appreciate your weighing in on two things: How can I test my changes in FieldBase.h to FieldBase<std::string>::get [Const] Reference in Python? I think the changes are necessary if this code is reachable at all, but all of my attempts to access a string field using reference notation failed. Is my change to error handling in BaseRecord.cc commit 0c65e9f acceptable? I think the old test was inadequate because it ignored the case that the left-hand side was fixed and the right-hand side variable.
            rowen Russell Owen made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            jbosch Jim Bosch added a comment -

            I'll do a full review, but I'm afraid the answer to your first question is that it is not possible to test the reference changes from Python; that would require (at least) a mutable string type in Python.

            Show
            jbosch Jim Bosch added a comment - I'll do a full review, but I'm afraid the answer to your first question is that it is not possible to test the reference changes from Python; that would require (at least) a mutable string type in Python.
            krughoff Simon Krughoff made changes -
            Sprint Alert Production S17 - 6 [ 616 ] Alert Production S17 - 6, Alert Production F17 - 7 [ 616, 626 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Hide
            jbosch Jim Bosch added a comment -

            Review complete. Comments on the GitHub PR.

            The changes you mentioned in your second question are certainly an improvement. It's possible we could improve that function further by adding support for mixed fixed- and variable-length copies, but I'm not sure we need to and it's definitely good to throw if we're not supporting it.

            Show
            jbosch Jim Bosch added a comment - Review complete. Comments on the GitHub PR. The changes you mentioned in your second question are certainly an improvement. It's possible we could improve that function further by adding support for mixed fixed- and variable-length copies, but I'm not sure we need to and it's definitely good to throw if we're not supporting it.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen Russell Owen added a comment -

            Thanks for the helpful comments. I adopted all suggested changes except one, as discussed on github.

            Show
            rowen Russell Owen added a comment - Thanks for the helpful comments. I adopted all suggested changes except one, as discussed on github.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            Hide
            rowen Russell Owen added a comment -

            I have discovered a misfeature with variable-length strings in C++: one cannot specify a new variable-length field unless one also specifies a value for the doReplace flag. E.g.

            # this works
            schema.addField<std::string>("foo", "docs for foo", 0, false)
             
            # this fails: the compiler says it is ambiguous:
            schema.addField<std::string>("foo", "docs for foo", 0)
            

            I'm not sure if there is anything reasonable that can be done about this. It is worrying because the compiler warning does not provide much help in figuring out the workaround.

            Show
            rowen Russell Owen added a comment - I have discovered a misfeature with variable-length strings in C++: one cannot specify a new variable-length field unless one also specifies a value for the doReplace flag. E.g. # this works schema.addField<std::string>("foo", "docs for foo", 0, false)   # this fails: the compiler says it is ambiguous: schema.addField<std::string>("foo", "docs for foo", 0) I'm not sure if there is anything reasonable that can be done about this. It is worrying because the compiler warning does not provide much help in figuring out the workaround.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Jim Bosch
                Watchers:
                Jim Bosch, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel