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

Stop using ndarray::EigenView indirectly in C++ code

    XMLWordPrintable

    Details

      Description

      Use the ndarray::Array functions added in DM-14728 instead of ndarray::Array::asEigen to eliminate our use of ndarray::EigenView in our C++ code.

      Warning: these new functions return Eigen::Map, which does not use reference counting to manage memory, so be careful when converting old code. In most instances nothing needs to change as the Eigen object will to out of scope before the ndarray array. However, there are likely to be a few bits of code that will require non-trivial changes in order to avoid memory leaks (e.g. if the Eigen Map is passed around or stored); in that case we have to copy the array data or keep the data as an ndarray::Array and delay extracting the Eigen version until deeper in the code.

      Note that updating our pybind11 wrappers will be done on a different ticket.

        Attachments

          Issue Links

          is blocked by

          Improvement - An improvement or enhancement to an existing feature or task. DM-14728 Add asEigenArray/Matrix returning Eigen::Map

          • Done
          relates to

          Improvement - An improvement or enhancement to an existing feature or task. DM-14725 Eliminate explicit use of ndarray::EigenView in C++ code

          • Done

            Activity

            Ascending order - Click to sort in descending order
            Hide
            Permalink
            rowen Russell Owen added a comment -

            This has passed Jenkins with the DN-14728 ticket branch of ndarray and so is ready for review, but not quite ready for merging (that branch needs a bit of cleanup before merging).

            Changes, where arr is an ndarray::Array:

            • arr::asEigen() -> ndarray::asEigenMatrix(arr)
            • arr::asEigen<Eigen::ArrayXpr> -> ndarray::asEigenArray(arr)
            • make sure arr stays in scope while the returned Eigen::Map is used, since the memory is managed by the arr object
            Show
            rowen Russell Owen added a comment - This has passed Jenkins with the DN-14728 ticket branch of ndarray and so is ready for review, but not quite ready for merging (that branch needs a bit of cleanup before merging). Changes, where arr is an ndarray::Array : arr::asEigen() -> ndarray::asEigenMatrix(arr) arr::asEigen<Eigen::ArrayXpr> -> ndarray::asEigenArray(arr) make sure arr stays in scope while the returned Eigen::Map is used, since the memory is managed by the arr object
            Hide
            Permalink
            krzys Krzysztof Findeisen added a comment -

            Looks good, aside from some outstanding style issues. I'm a bit disappointed that we need to pay such close attention to temporaries, but I don't see a good solution within the constraints we have.

            Show
            krzys Krzysztof Findeisen added a comment - Looks good, aside from some outstanding style issues. I'm a bit disappointed that we need to pay such close attention to temporaries, but I don't see a good solution within the constraints we have.
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Thank you for the helpful review. I implemented most of your requested changes, but declined a few with suitable explanations on github. Regarding the requested cleanup of one line in meas_modelfit I'm happy to file a ticket if you feel strongly about this.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful review. I implemented most of your requested changes, but declined a few with suitable explanations on github. Regarding the requested cleanup of one line in meas_modelfit I'm happy to file a ticket if you feel strongly about this.

              People

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

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.