DM-14740: Stop using ndarray::EigenView indirectly in C++ code #360
Conversation
and thus stop directly using ndarray::EigenView. Use ndarray::asEigenMatrix instead of ndarray::Array::asEigen
There was a problem hiding this comment.
I don't think I understand this code well enough to confirm that it's correct -- it seems like the lifetime of LeastSquares's matrix/vector data is a bit ill-defined. @r-owen, can you take a look at my questions so far? Maybe things will become clearer then...
| @@ -223,7 +223,7 @@ class LeastSquares { | |||
| * by future calls to LeastSquares member functions, so it's best to promptly copy the | |||
| * result elsewhere. | |||
| * | |||
| * If you want an Eigen object instead, just use getSolution().asEigen(). | |||
| * If you want an Eigen object instead, just use ndarray::asEigenMatrix(getSolution()). | |||
There was a problem hiding this comment.
👍 for updating the documentation.
| _getDesignMatrix() = design.asEigen().template cast<double>(); | ||
| _getDataVector() = data.asEigen().template cast<double>(); | ||
| _getDesignMatrix() = ndarray::asEigenMatrix(design).template cast<double>(); | ||
| _getDataVector() = ndarray::asEigenMatrix(data).template cast<double>(); |
There was a problem hiding this comment.
What if design and data are temporaries? Or is cast<double> guaranteed to make a defensive copy (which would also explain how the parameters can be const)? I looked up the documentation and it didn't say anything about object independence.
Same question about other methods that use this implementation pattern.
There was a problem hiding this comment.
I hope @TallJimbo will say more, but my understanding is that this makes a copy because the target is a (reference to) an Eigen::MatrixXd
There was a problem hiding this comment.
The assignment operation is where the (only) copy is made. The call to cast is safe only because temporaries returned by asEigenMatrix are not destroyed until the end of their statements (see e.g. https://stackoverflow.com/questions/3041249/when-are-temporaries-created-as-part-of-a-function-call-destroyed).
There was a problem hiding this comment.
Ah, ok, I didn't realize the assignment copied the data. Maybe I've spent too long staring at ndarray and afw::image... 🙃
| @@ -140,7 +140,7 @@ std::shared_ptr<GaussianPsf::Image> GaussianPsf::doComputeKernelImage(lsst::geom | |||
| sum += row[xIndex] = std::exp(-0.5 * (x * x + y * y) / (_sigma * _sigma)); | |||
| } | |||
| } | |||
| array.asEigen() /= sum; | |||
| ndarray::asEigenMatrix(array) /= sum; | |||
There was a problem hiding this comment.
This looks a bit weird. How is ndarray::asEigenMatrix an lvalue? I found the definition, but the return value doesn't look like a reference.
There was a problem hiding this comment.
Again, @TallJimbo may be able to explain it better, but I believe the fact that it is an Eigen::Map means it can be read or written. It's just a view into array data.
There was a problem hiding this comment.
Correct. Eigen::Map is basically a "smart reference", something C++ is notoriously bad at providing intuitive syntax for.
No description provided.