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

Enhance afw table to support variable-length string data

    XMLWordPrintable

    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

          blocks

          Improvement - An improvement or enhancement to an existing feature or task. DM-10765 Replace existing WCS classes with SkyWcs

          • Done

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            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.
            Hide
            Permalink
            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.
            Hide
            Permalink
            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.
            Hide
            Permalink
            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.
            Hide
            Permalink
            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:

                  CI Builds

                  No builds found.