Closed eddelbuettel closed 2 years ago
I took another look, and I think I understand better what is happening. While Armadillo allow 1d, 2d, and 3d Field creators:
these are in fact always represented in '3d' i.e. attributes rows, cols, slices are present.
In the corner cases of an F(1)
, all three are of values 1, and n_elem
is also one. So yep what the PR does is correct: setting up a 3d dim
objects with <rows, cols, slizes> is exactly what we wanted. And which the old code gets wrong.
This is now documented in new unit test code (for both without (i.e. default "old" behaviour) and with (i.e. "new" behaviour).
I will try to complement the unit test files with some for the opposite direction, i.e. R to C++.
Yes, they always have rows, cols, and slices. This is somewhat confusing.
I'll have a look into it and I'm happy to add some tests. Probably around mid-next week.
I now added a complete (if hard to read) set. It demonstrates where the old (current) code is wrong or difficient -- now tests have comments or are commented out -- and the new tests show that everything is as expected.
Nice work, again, in your PR. We'll work on transitioning to making it the default "eventually".
Following #352 I started to add a unit test file in C++ (and a caller in R) for fields. This could (and should) get expanded. My plan so far is to have test for both import (none so far) and exports (started).
If I could twist your arm @BerriJ to maybe contribute a few for where you did saw the failures pre-PR ? We can possiblty do an
expect_error()
around it.This can also serve as extra documentation as it shows the usage.