-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Writing Eigen::MatrixXd
as column major.
#532
Comments
Thanks for pointing this out! What I do in H5Easy is to cast to row-major before writing
|
Casting it like that seems like a good idea (element of least surprise)! Should this be unified / lifted to "core" to avoid duplication? And would it be wise to make it an option to throw an error / assert if a type conversion happens? Thinking of bigger matrices that one wouldn't really want to store in memory multiple times. |
@matz-e I must admit that casting was in part laziness that we can fix if we fix |
Are there any plans to fix the bug when writing column major matrices? |
There's few things that make this a difficult decision:
Therefore, I'm not sure it's a good idea to fix it during the That said, it's one of several things that need fixing in HighFive. The others being:
Both the CMake and serialization of strings would be much easier to fix if we could break API slightly. Hence, they might all get bundled, delayed until all of them have been fixed. If you're using Eigen, maybe just avoid HighFive all together. OTOH that's a bit radical, maybe use it to create the groups, datasets and datatypes; but don't use it to write/read data from/to |
Well... If I'm not mistaken I think that the H5Easy interface does read/write correctly (it casts to row-major before writing, or from row-major after reading). So it is not so trivial to say that one should not fix this bug directly to be backward compatible. What if one wrote with H5Easy and now reads with the core API? |
Yes, this is just another variation of reading data written by a different library; or from/to something that isn't an |
Yes, I see. This would lead to a huge break of existing code. |
For reference HighFive/include/highfive/h5easy_bits/H5Easy_Eigen.hpp Lines 88 to 90 in be68bd0
HighFive/include/highfive/h5easy_bits/H5Easy_Eigen.hpp Lines 134 to 138 in be68bd0
I don't care because I don't use the defective code. However, I would like to point out that purposefully keeping a bug seems unwise: people might be creating new 'corrupted' data that might lead to (undetected?) errors when reading with other libraries (e.g. h5py) - which is not unlikely as having binding for many languages and programs is one of the key features of HDF5. I would rather think about a scheme with a runtime warning that can be silenced and a(n) (pre-processor) option to fall back on the current incorrect behaviour. Regardless on what default is kept until the next major update, the warning should really be there ASAP IMO. Having an option to interpret col-major data row-major and inverse could even be considered a valid overload for permanent use. Though I'm not a big fan unless we find a use-case. |
Very nice write up of the alternative chain of reasoning. Realistically, I don't see this getting fixed soon, as unpleasant as that may be. What can be done with little effort is to break it completely. It's already broken, we would simply make it break loudly. |
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
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
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
Eigen support has been fixed/re-implemented in #968 for v3 and onwards. To find out if a particular codebase using v2.7.x and earlier is affected, first update to v2.8 or v2.9. Then run an suitably extensive test suite, or application runs. If the code tries to write or read any 2D Eigen objects, in a faulty manner, it will throw. Eigen support in v3 includes row- and colum-major arrays; and maps thereof. Not (yet) supported are strided maps and padded arrays. |
I've created a PR for adding an example which writes an
Eigen::MatrixXd
, see #531. It's supposed to create a matrixThis matrix is then written to file. When dumping the resulting file I get
Which is consistent with HDF5 assuming data is row-major (C-style) and the actual Eigen matrix being column-major (Fortran/BLAS-style). I couldn't immediately find any code that transposes the matrix before writing it.
Note that a write-read cycle using HighFive returns, as expected, the same matrix.
Can someone confirm that I've set up the example properly and maybe comment on the desired behaviour?
The text was updated successfully, but these errors were encountered: