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

Stop transposing data in Mapping.tranForward and tranInverse

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: astshim
    • Story Points:
      6
    • Sprint:
      Alert Production S17 - 3, Alert Production S17 - 4, Alert Production S17 - 5
    • Team:
      Alert Production

      Description

      I originally designed astshim to use a transposed version of data lists because we expected to be able to provide that with live views of PointList and SpherePointList (and we had some hope AST could be updated to support transposed data directly). Both of these have proven false, so it makes much more sense to use AST's standard data order in astshim. This will be more efficient (it avoids a copy on input and output) and will be less surprising to users.

        Attachments

          Activity

          Hide
          rowen Russell Owen added a comment -

          Changes include in astshim:

          • stop transposing data in C++
          • update unit tests to transpose input data and expected outputs
          • update unit tests to unify variable names for input and output data (a separate commit)
            and in afw:
          • stop transposing data in C++ Endpoint
          • stop wrapping Point3Endpoint because we have no use case for it and it would require extra code to support (which can easily be added later if a need is found). Formerly it required no extra code.
          • update unit tests
          Show
          rowen Russell Owen added a comment - Changes include in astshim: stop transposing data in C++ update unit tests to transpose input data and expected outputs update unit tests to unify variable names for input and output data (a separate commit) and in afw: stop transposing data in C++ Endpoint stop wrapping Point3Endpoint because we have no use case for it and it would require extra code to support (which can easily be added later if a need is found). Formerly it required no extra code. update unit tests
          Hide
          nlust Nate Lust added a comment -

          Overall looks good to me, a few minor comments up on github. Nothing show stopping.

          Show
          nlust Nate Lust added a comment - Overall looks good to me, a few minor comments up on github. Nothing show stopping.
          Hide
          rowen Russell Owen added a comment -

          I forgot to make a pull request for afw. I've now done that. Please have a look.

          Show
          rowen Russell Owen added a comment - I forgot to make a pull request for afw. I've now done that. Please have a look.
          Hide
          rowen Russell Owen added a comment - - edited

          I got rid of Point3Endpoint because we have no identified need for it and adding it requires additional code (which was not the case when raw data was transposed). However, for the record, here are some ideas for adding back later (or if Nate Lust or others strongly prefer, on this ticket branch):

          • Add a constructor to Point<T, N> that takes a column view of an ndarray. This is easy to use correctly and efficient, but does add another constructor to the API of Point<T, N>.
          • Add a "strides" argument to the raw data pointer constructor that defaults to 1. This was Jim Bosch's suggestion. it is very simple to implement and adds a minimum of API clutter, but I worry that it is too easy to get the value of strides wrong.
          • Add two new classes: Point3Endpoint and template<N> BasePointEndpoint, to hold code common to Point2Endpoint and Point3Endpoint. This needs no changes to the Point classes but I feel it is a bit too messy.
          • Copy the data for each point from the 2D raw array into a C array, then use the C array to construct the Point. Krzysztof Findeisen suggested this. It is very simple but requires an extra copy for each axis of each point. I worry about the performance hit, though it would surely be small and perhaps negligible.
          Show
          rowen Russell Owen added a comment - - edited I got rid of Point3Endpoint because we have no identified need for it and adding it requires additional code (which was not the case when raw data was transposed). However, for the record, here are some ideas for adding back later (or if Nate Lust or others strongly prefer, on this ticket branch): Add a constructor to Point<T, N> that takes a column view of an ndarray. This is easy to use correctly and efficient, but does add another constructor to the API of Point<T, N> . Add a "strides" argument to the raw data pointer constructor that defaults to 1. This was Jim Bosch 's suggestion. it is very simple to implement and adds a minimum of API clutter, but I worry that it is too easy to get the value of strides wrong. Add two new classes: Point3Endpoint and template<N> BasePointEndpoint , to hold code common to Point2Endpoint and Point3Endpoint . This needs no changes to the Point classes but I feel it is a bit too messy. Copy the data for each point from the 2D raw array into a C array, then use the C array to construct the Point. Krzysztof Findeisen suggested this. It is very simple but requires an extra copy for each axis of each point. I worry about the performance hit, though it would surely be small and perhaps negligible.
          Hide
          nlust Nate Lust added a comment -

          Nothing caught my eye as worth addressing in the afw branch, so the only comments I have are for the other one. One minor note is that officially all comments are supposed to be complete sentences with punctuation etc, but I don't think it is worth taking a second look at.

          Show
          nlust Nate Lust added a comment - Nothing caught my eye as worth addressing in the afw branch, so the only comments I have are for the other one. One minor note is that officially all comments are supposed to be complete sentences with punctuation etc, but I don't think it is worth taking a second look at.

            People

            • Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Nate Lust
              Watchers:
              Krzysztof Findeisen, Nate Lust, Russell Owen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel