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

Eliminate explicit use of ndarray::EigenView in C++ code

    Details

      Description

      ndarray::EigenView relies on undocumented internals of Eigen. Stop using it, in order to allow upgrading Eigen.

      The full conversion consists of two parts:
      1) Stop using ndarray::EigenView explicitly in C++ code. That is what this ticket is about.
      2) Stop using ndarray::EigenView indirectly via ndarray::Array::asEigen by having that function return an Eigen::Map. That is DM-14728

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-10068 [ 31628 ]
            rowen Russell Owen made changes -
            Link This issue relates to RFC-479 [ RFC-479 ]
            rowen Russell Owen made changes -
            Risk Score 0
            rowen Russell Owen made changes -
            Link This issue blocks DM-14305 [ DM-14305 ]
            rowen Russell Owen made changes -
            Summary Eliminate usage of ndarray::EigenView Eliminate use of ndarray::EigenView in C++ code
            rowen Russell Owen made changes -
            Description ndarray::EigenView relies on undocumented internals of Eigen. Stop using it, in order to allow upgrading Eigen. ndarray::EigenView relies on undocumented internals of Eigen. Stop using it, in order to allow upgrading Eigen.

            The full conversion consists of two parts:
            1) Stop using {{ndarray::EigenView}} explicitly in C++ code. That is what this ticket is about.
            2) Stop using {{ndarray::EigenView}} indirectly via {{ndarray::Array::asEigen}} by having that function return an {{Eigen::Map}}. I'll file a separate ticket for that. It will be tricky because we have to be sure memory is being managed properly for all the objects returned by {{asEigen}}.
            rowen Russell Owen made changes -
            Link This issue relates to DM-14728 [ DM-14728 ]
            rowen Russell Owen made changes -
            Summary Eliminate use of ndarray::EigenView in C++ code Eliminate explicit use of ndarray::EigenView in C++ code
            rowen Russell Owen made changes -
            Story Points 10 2
            rowen Russell Owen made changes -
            Story Points 2 1
            rowen Russell Owen made changes -
            Sprint AP S18-6 [ 686 ]
            rowen Russell Owen made changes -
            Description ndarray::EigenView relies on undocumented internals of Eigen. Stop using it, in order to allow upgrading Eigen.

            The full conversion consists of two parts:
            1) Stop using {{ndarray::EigenView}} explicitly in C++ code. That is what this ticket is about.
            2) Stop using {{ndarray::EigenView}} indirectly via {{ndarray::Array::asEigen}} by having that function return an {{Eigen::Map}}. I'll file a separate ticket for that. It will be tricky because we have to be sure memory is being managed properly for all the objects returned by {{asEigen}}.
            ndarray::EigenView relies on undocumented internals of Eigen. Stop using it, in order to allow upgrading Eigen.

            The full conversion consists of two parts:
            1) Stop using {{ndarray::EigenView}} explicitly in C++ code. That is what this ticket is about.
            2) Stop using {{ndarray::EigenView}} indirectly via {{ndarray::Array::asEigen}} by having that function return an {{Eigen::Map}}. That is DM-14728
            rowen Russell Owen made changes -
            Reviewers Krzysztof Findeisen [ krzys ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            krzys Krzysztof Findeisen added a comment - - edited

            In standup you mentioned something about pybind11 conversion being done by Eigen vs. by ndarray. Does the text at https://developer.lsst.io/pybind11/how-to.html#ndarray need to be changed?

            Show
            krzys Krzysztof Findeisen added a comment - - edited In standup you mentioned something about pybind11 conversion being done by Eigen vs. by ndarray. Does the text at https://developer.lsst.io/pybind11/how-to.html#ndarray need to be changed?
            Hide
            rowen Russell Owen added a comment -

            When we stop using EigenView in pybind11 wrappers then yes, that text will have to be changed (along with all the pybind11 wrappers that use the old technique). However, that work will be done on a different ticket. Initially I am focused on converting our C++ code (excluding pybind11 wrappers) away from ndarray::EigenView. Once that is done we can switch our pybind11 wrappers to use pybind11's built-in Eigen support, which will eliminate the last usage of ndarray::EigenView and (once ndarray is updated to remove EigenView) allow upgrading Eigen.

            Show
            rowen Russell Owen added a comment - When we stop using EigenView in pybind11 wrappers then yes, that text will have to be changed (along with all the pybind11 wrappers that use the old technique). However, that work will be done on a different ticket. Initially I am focused on converting our C++ code (excluding pybind11 wrappers) away from ndarray::EigenView. Once that is done we can switch our pybind11 wrappers to use pybind11's built-in Eigen support, which will eliminate the last usage of ndarray::EigenView and (once ndarray is updated to remove EigenView) allow upgrading Eigen.
            Hide
            krzys Krzysztof Findeisen added a comment -

            I suggest a slightly different implementation in meas_modelfit. Otherwise looks good.

            Show
            krzys Krzysztof Findeisen added a comment - I suggest a slightly different implementation in meas_modelfit . Otherwise looks good.
            krzys Krzysztof Findeisen made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Link This issue relates to DM-14740 [ DM-14740 ]
            swinbank John Swinbank made changes -
            Epic Link DM-10068 [ 31628 ] DM-14447 [ 80385 ]

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Krzysztof Findeisen
                Watchers:
                Jim Bosch, John Parejko, Krzysztof Findeisen, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: