Details
-
Type:
Improvement
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ndarray
-
Labels:
-
Story Points:10
-
Epic Link:
-
Team:Alert Production
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
| Field | Original Value | New Value |
|---|---|---|
| Epic Link |
|
| Risk Score | 0 |
| Story Points | 20 | 10 |
| Component/s | afw [ 10714 ] | |
| Component/s | meas_astrom [ 10745 ] | |
| Component/s | ndarray [ 10761 ] |
| Summary | Change ndarray asEigen to return Eigen::Map instead of ndarray::EigenView | Add asEigenArray/Matrix returning Eigen::Map |
| Component/s | afw [ 10714 ] | |
| Component/s | meas_astrom [ 10745 ] |
| Description |
Change {{ndarray::Array::asEigen}} to return {{Eigen::Map}} instead of {{ndarray::EigenView}} and update the DM stack accordingly.
The primary concern is that {{EigenView}} manages its memory and {{Eigen::Map}} does not so we have to be careful not to introduce memory leaks. |
Add {{ndarray::Array::asEigenArray}} and {{asEigenMatrix}} 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}}. |
| Assignee | Russell Owen [ rowen ] | Jim Bosch [ jbosch ] |
| Description |
Add {{ndarray::Array::asEigenArray}} and {{asEigenMatrix}} 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}}. |
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}}. |
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.
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).
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.
| Assignee | Jim Bosch [ jbosch ] | Russell Owen [ rowen ] |
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.
| Status | To Do [ 10001 ] | In Progress [ 3 ] |
(Just to be clear, I think that was a miscommunication — the correct epic for this work is DM-14447).
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.
| Reviewers | Jim Bosch [ jbosch ] | |
| Status | In Progress [ 3 ] | In Review [ 10004 ] |
| Resolution | Done [ 10000 ] | |
| Status | In Review [ 10004 ] | Done [ 10002 ] |
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:
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.