Remove Eigen functionality for matrices. #731
Conversation
HighFive hasn't been respecting that Eigen uses Fortran order, as a result it's been writing `Eigen::Matrix` to file as if the data were C order. It's been consistently reading datasets back into `Eigen::Matrix` has if it already was in Fortran order. This error is symmetric, i.e., if one writes an `Eigen::Matrix` to disk via core HighFive and then reads that dataset back into an `Eigen::Matrix` the outcome is correct (which has been check in CI). More information can be found at: #532
Codecov Report
@@ Coverage Diff @@
## master #731 +/- ##
==========================================
+ Coverage 82.43% 82.70% +0.26%
==========================================
Files 67 66 -1
Lines 4514 4515 +1
==========================================
+ Hits 3721 3734 +13
+ Misses 793 781 -12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| "HighFive has been broken for Eigen::Matrix. Please check " | ||
| "https://github.com/BlueBrain/HighFive/issues/532."); |
There was a problem hiding this comment.
| "HighFive has been broken for Eigen::Matrix. Please check " | |
| "https://github.com/BlueBrain/HighFive/issues/532."); | |
| "HighFive has been broken for Eigen::Matrix. " | |
| "Please use H5Easy or open a PR. See " | |
| "https://github.com/BlueBrain/HighFive/issues/532."); |
There was a problem hiding this comment.
It's unfortunately not that simple. Just switching to H5Easy isn't going to fix the issue of existing files. The point of breaking this is to make people check the ticket so that they understand how they're affected by this issue.
There was a problem hiding this comment.
You could say "Please use H5Easy to write NEW data. Note that old data may be 'corrupted'"
There was a problem hiding this comment.
I'm not sure I understand what you want to achieve by suggestion H5Easy as an alternative, it's not a drop in replacement (Vectors serialized from core are 2D; while Easy strips the singleton dimension).
I would like the message to read that they need to go read a full length piece of text; and then figure out how this affects them.
The point is to end the indecision.
There was a problem hiding this comment.
It's fine, you can keep what you had.
| // and write it to a file | ||
| int main(void) { | ||
| try { | ||
| Eigen::MatrixXd matrix(nrows, ncols); |
There was a problem hiding this comment.
In stead of removing, you could use a row-major example for the moment. We can then revert one someone had the time to fix HighFive
There was a problem hiding this comment.
No, git revert works quite well. I don't want there to be anything suggesting that reading or writing Eigen::Matrix from core HighFive is sensible.
| const int ncols = 3; | ||
|
|
||
| try { | ||
| Eigen::MatrixXd mat(nrows, ncols); |
| dset.write(mat); | ||
|
|
||
| Eigen::MatrixXd result; | ||
| dset.read(result); |
There was a problem hiding this comment.
It would always be good to add an assertion
There was a problem hiding this comment.
I don't think I understand. What should be asserted here? This is an example we presented in a poster as PASC.
There was a problem hiding this comment.
Just a detail that caught my eye. I just wanted to say just to make sure that example never diverge an assertion would not have hurt ;). But indeed, never mind
|
@tdegeus Thank you for your time and comments! |
HighFive hasn't been respecting that Eigen uses Fortran order, as a result it's been writing
Eigen::Matrixto file as if the data were C order. It's been consistently reading datasets back intoEigen::Matrixhas if it already was in Fortran order.This error is symmetric, i.e., if one writes an
Eigen::Matrixto disk via core HighFive and then reads that dataset back into anEigen::Matrixthe outcome is correct (which has been check in CI).More information can be found at:
#532