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

Stop transposing data in Mapping.tranForward and tranInverse

    XMLWordPrintable

Details

    • Improvement
    • Status: Done
    • Resolution: Done
    • None
    • astshim
    • 6
    • Alert Production S17 - 3, Alert Production S17 - 4, Alert Production S17 - 5
    • 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

          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
          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
          nlust Nate Lust added a comment -

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

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

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

          rowen Russell Owen added a comment - I forgot to make a pull request for afw. I've now done that. Please have a look.
          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 nlust 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 jbosch'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. krzys 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.
          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 nlust 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 jbosch '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. krzys 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.
          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.

          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

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

            Dates

              Created:
              Updated:
              Resolved:

              Jenkins

                No builds found.