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

Add asEigenArray/Matrix returning Eigen::Map

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ndarray

      Description

      Add ndarray::asEigenArray(array) and asEigenMatrix(array) which return Eigen::Map.

      Also make it possible (or automatic) to eliminate ndarray::Array::asEigen and ndarray::EigenView when building against a more modern version of Eigen.

      This is to help LSST upgrade Eigen, which will probably require eliminating our use of ndarray::EigenView.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            Thank you for implementing this. Can you please say more about why you emulated ndarray::Array::asEigen such that asEigen(array) returns a Matrix map and ndarray::asEigen<Eigen::ArrayXpr>(array) returns an Array map.

            I had been hoping for asEigenArray and asEigenMatrix for two reasons:

            • It makes the code easier to read and more obvious (it's not clear to me why asEigen would return a Matrix instead of an Array)
            • It makes it a bit easier to spot old LSST code that needs conversion

            On the other hand if one can request more than just Matrix and Array then the flexibility may be helpful and we do have a few other asEigen functions that will confuse conversion regardless of what we call this.

            Show
            rowen Russell Owen added a comment - Thank you for implementing this. Can you please say more about why you emulated ndarray::Array::asEigen such that asEigen(array) returns a Matrix map and ndarray::asEigen<Eigen::ArrayXpr>(array) returns an Array map. I had been hoping for asEigenArray and asEigenMatrix for two reasons: It makes the code easier to read and more obvious (it's not clear to me why asEigen would return a Matrix instead of an Array) It makes it a bit easier to spot old LSST code that needs conversion On the other hand if one can request more than just Matrix and Array then the flexibility may be helpful and we do have a few other asEigen functions that will confuse conversion regardless of what we call this.
            Hide
            jbosch Jim Bosch added a comment -

            There have a been a few occasions where I've wanted to write code that works with either Array or Matrix objects, and I hate having to duplicate that code rather than just templating it.  I wouldn't object to also having asEigenMatrix and asEigenArray (or just the latter), but I'd like to keep the current new signatures, too.

            Show
            jbosch Jim Bosch added a comment - There have a been a few occasions where I've wanted to write code that works with either Array or Matrix objects, and I hate having to duplicate that code rather than just templating it.  I wouldn't object to  also having asEigenMatrix and asEigenArray (or just the latter), but I'd like to keep the current new signatures, too.
            Hide
            rowen Russell Owen added a comment -

            Fair enough. How about offering asEigenMatrix and asEigenVector specializations and removing the default for that template parameter for bare asEigen?

            I realize it's a subtle difference from the old Array::asEigen, but based on reading old code using that API I think it would improve readability and discoverability. (Also the old API is going away, at least for LSST when using newer Eigen).

            Show
            rowen Russell Owen added a comment - Fair enough. How about offering asEigenMatrix and asEigenVector specializations and removing the default for that template parameter for bare asEigen ? I realize it's a subtle difference from the old Array::asEigen, but based on reading old code using that API I think it would improve readability and discoverability. (Also the old API is going away, at least for LSST when using newer Eigen).
            Hide
            jbosch Jim Bosch added a comment -

            Russell Owen, as you're doing most of the work on this, you might as well take it back. You may also want to take a look at whether the Epic is right - either it isn't, or the Epic's description has the wrong cycle.

            Show
            jbosch Jim Bosch added a comment - Russell Owen , as you're doing most of the work on this, you might as well take it back. You may also want to take a look at whether the Epic is right - either it isn't, or the Epic's description has the wrong cycle.
            Hide
            rowen Russell Owen added a comment - - edited

            Jim Bosch did the heavy lifting adding the free functions asEigen but suggested I take it back due to doing some of the work, so I did so.

            As to the epic: it is the one John Swinbank suggests we use. Perhaps the epic needs updating.

            Show
            rowen Russell Owen added a comment - - edited Jim Bosch did the heavy lifting adding the free functions asEigen but suggested I take it back due to doing some of the work, so I did so. As to the epic: it is the one John Swinbank suggests we use. Perhaps the epic needs updating.
            Hide
            swinbank John Swinbank added a comment -

            (Just to be clear, I think that was a miscommunication — the correct epic for this work is DM-14447).

            Show
            swinbank John Swinbank added a comment - (Just to be clear, I think that was a miscommunication — the correct epic for this work is DM-14447 ).
            Hide
            rowen Russell Owen added a comment -

            I updated the tarball, tweaked the build to build EigenView (since it will take a bit of work on our wrappers to switch to pybind11's Eigen header) and removed an outdated patch.

            Show
            rowen Russell Owen added a comment - I updated the tarball, tweaked the build to build EigenView (since it will take a bit of work on our wrappers to switch to pybind11's Eigen header) and removed an outdated patch.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: