kaskr / adcomp

AD computation with Template Model Builder (TMB)
Other
176 stars 80 forks source link

Add row method to array #313

Open 2039 opened 4 years ago

2039 commented 4 years ago

I found it easier to do array.row(n) than array.transpose().col(n).

Note: I'm inexperienced in C/C++. I've only tested this on my own program, and it appears to be working, but it should preferably be reviewed first. Please let me know of any mistakes so I can correct them.

kaskr commented 4 years ago

@2039 Thanks for the suggestion, but unfortunately it's not that simple to add a row method - at least not with the current array design.

To be consistent with other classes (vectors and matrices) the array extractors must provide write access. See e.g. here: http://kaskr.github.io/adcomp/matrix_arrays_8cpp-example.html

a1.col(1) = v1;  // assign v1 to 2nd col of a1
REPORT(a1);

This only works for array columns because columns are contiguous segments of the underlying data.

2039 commented 4 years ago

Unless I'm missing something, arrays are already inconsistent with matrices

array<int> A(2,2);
A << 1, 2, 3, 4;
A.transpose() << 5, 6, 7, 8;
std::cout << "A:\n" << A << std::endl;
/*
A:
1
2
3
4
*/

matrix<int> M(2,2);
M << 1, 2, 3, 4;
M.transpose() << 5, 6, 7, 8;
std::cout << "M:\n" << M << std::endl;
/*
M:
5 6
7 8
*/

introducing more inconsistencies should probably be avoided, but I'd argue that the lack of a row accessor is an inconsistency by itself.

kaskr commented 4 years ago

It's true there are some inconsistencies in the array class that would be nice to fix. However, what I'm saying is that it's not so easy with the current design. I'd be happy to consider a row extractor if you make it in a way that triggers a compile time error if a user tries to assign directly to the array row. Likewise, it would be nice to fix the array transpose() caveat you pointed out, either by implementing the assignment method, or by generating a compile time error if used.