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

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

    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

            Activity

            Hide
            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
            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
            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: