rootext: Reimplement MatOp in pure C++#53
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #53 +/- ##
==========================================
- Coverage 63.07% 62.83% -0.24%
==========================================
Files 70 69 -1
Lines 10066 9870 -196
==========================================
- Hits 6349 6202 -147
+ Misses 3717 3668 -49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| // Write | ||
| for (int32_t c = 0; c < cc; c++) { | ||
| if (mput_(dst_matrix, &buffer[c], level, column + c, line, ll) != ll) { |
There was a problem hiding this comment.
Same as above, this is reading from overlapping chunks of buffer.
|
It seems the data from mfile-samples I used were quite limited. But I don't have better files for testing |
|
I got a bigger matrix from @h-mayr, tried it and the matrix transpose failed. Digging into it, I found that the new code is in theory correct, BUT the default matrix format is Using I'll look deeper into it :( left: original matrix, center: old code, right: new code but with |
fee2059 to
0546bef
Compare
|
-> Fixed previous issues I've reimplemented I've tested the code with the matrix from the last comment. Now the md5 hashes of the generated files matches those of the original code. Also when reducing |
|
I think it would be best to add some unit or component tests to verify the behavior of MatOp |
|
I could add A/B testing if the code produces the same results as before with known matrices. Aka, adding the test I've done manually to pytest. I found no easy way to generate test files within pytest. How should I continue here? |
|
I get more and more confused about the existing code I've added a test code and the original code shoes some unexpected behavior (left: the input (73x172), middle: the result with the old code, right: the new code / my expectation / caution: the axis scaling is a bit confusing here) @janmayer @nfbraun: Could you explain why the code behaves this way? |
|
Ok, I digged deeper. The old transpose code produces a matrix with the right size, but the matrix is not fully transposed. In the matrix from above, lines 74-126 are 0. So is this intended behavior in the old code or a bug?
~> ./mtx_info ../../a.mtx
Matrix: ../../a.mtx
Filetype: 8
Levels: 1
Lines: 73
Columns: 127
Version: 2
Status: 0
Name: ../../a.mtx
Comment: (null)
~> ./mtx_info ../../a.tmtx
Matrix: ../../a.tmtx
Filetype: 8
Levels: 1
Lines: 127
Columns: 73
Version: 2
Status: 0
Name: ../../a.tmtx
Comment: (null) |
|
@op3 Can you have a look at this PR again? Thanks |
I could not find anybody who can explain why the behaviour of the old code should be a feature. To me it looks like a bug and I would suggest we treat it as such. I am not sure, if the transposing functionality is even used. |
When loading a file via |
MatOp.cc implements projection and transposing of mfile based matrices with the help of functions derived from the matop package. Reimplement those two functions directly in C++ to reduce code size and complexity. Furthermore, this helps to prepare the rootext code to be loaded via the ROOT CLING interpreter.
Currently there is no interface that allows yout to generate mfiles on the fly with Python. Therefore all testing data must be checked-in as blobs. To limit the file sizes of these blobs, only two small matrices and their transposes are checked in. These matrices were generated using `create_mtx.c`. This code can also generate larger matrices to test possible edge cases in the MatOp.Transpose implementation.
|
I've reordered the commits to not have a state where the tests fail. So first the new Implementation with the "fix" is applied and then the tests for the correct behavior are added. |
3 participants
This patch is a step in a upcoming series to reduce the C/C++ code in HDTV and make it either be precompiled (mfile, with no ROOT dependency) or load via the ROOT CLING interpreter.
Commit Message:
MatOp.cc implements projection and transposing of mfile based matrices with the help of functions derived from the matop package.
Reimplement those two functions directly in C++ to reduce code size and complexity. Furthermore, this helps to prepare the rootext code to be loaded via the ROOT CLING interpreter.
The new implementation was A/B tested against the original implementation with the matrices in src/hdtv/rootext/mfile-root/mfile/spec-samples: 1k.hf4, 1k.hf8, 1k.lf4, 1k.lf8, 1k.txt.
hdtv> matrix get asym src/hdtv/rootext/mfile-root/mfile/spec-samples/1k.*
The generated files were the same.
The transpose of 1k.vaxf and 1k.vaxg failed both, before and after this patch.
Testing
The testsuite does not cover loading and generating the projections and transformations of mfile matrices. A/B testing was done manually with the limited fies in the mfile-samples directory.
Copyright
The original matop/matop_*** C code was copyrighted under the
BSD-3-Clauselicense. The HDTV code is underGPL-2.0-or-later. I'm unsure what the correct way here is. Part of the new core is inherited from the BSD code, some is new, some has a strong influence.@h-mayr: This patch touches the behavior of HDTV, even if it should not change anything.