Rarray rebuild
#610
Rarray rebuild
#610
Conversation
|
Yes! I support this fully. I also hope it has multiple dimension support. |
|
Can you request reviews from me / us when you're ready? |
It seems ready for me, but I can't (or don't know how to) ask for you to be a reviewer... I tried to do as told here but it seems I am not able to do it. |
|
Maybe there is some issue with access that you cannot ask for reviews? What about unit tests? Did you manage to make it work with old ones or do we have no tests for |
I added some tests but I don't know if it covers type conversions... Could you be more specific on what these should be? |
extendr-api/src/wrapper/array.rs
Outdated
| let prod = dims_ref.iter().product::<usize>(); | ||
| if prod != robj.len() { | ||
| return Err(Error::Other(format!( | ||
| "The vector length ({}) does not match the length implied by the dimensions ({})", |
There was a problem hiding this comment.
I saw this error twice in the codebase, I believe it deserves its own error type with perhaps two parameters and this string moved to impl Debugimpl Display.
There was a problem hiding this comment.
I was trying to implement a "more generic" ValuesMismatch error where it accepts 2 values that implement the Debug trait + a String but the PartialEq derive for Error blocked it... I think it would be better to create an issue in order to implement it another time...
I implemented a DimensionMismatch error anyway, but I ended up not too happy about it since it is too specialized...
| // Create 2D RArray. | ||
| let slice = [1,2,3,4,5,6]; | ||
| let dim = [2,3]; | ||
| let array = RArray::from_slice(slice, dim).unwrap(); |
There was a problem hiding this comment.
Are there other construction methods besides from_slice()?
There was a problem hiding this comment.
There's also from_slice_unchecked. Since creating a new RArray creates a copy of the inner data, these were the only 2 methods I thought were needed. Note that you can use from_slice() directly on a Vec or on a rust array in this case.
I made RArray::new() available only to be used internally, since it doesn't add the dim attribute for the Robj.
Let me know if you are thinking of another type of constructor.
There was a problem hiding this comment.
Is there a way to create one without copying data? That would be quite nice. Additionally, it would be handy to be able to determine the col/row major order or at least provide a closure similar to the matrix method.
There was a problem hiding this comment.
I don't think it is possible to create an Robj without copying data.
Is it ok for you if I add an example on how to build row major matrices? Or is the RMatrix constructor using a closure really desirable? I removed it because think the data manipulation should be left for the user to figure out and also because it was a somewhat complicated function to understand how to use...
Maybe @CGMossa and @Ilia-Kosenkov can give their opinion on this.
#[test]
fn test_collect_rarray_matrix_row_major() {
test! {
// Test building an RMatrix in row major order.
let v: Vec<i32> = (1i32..=12).collect();
let nrow = 4_usize;
let ncol = 3_usize;
let rmat = (0..ncol)
.flat_map(|i| {
v.iter()
.skip(i)
.step_by(ncol)
.cloned()
})
.collect_rarray([nrow, ncol]);
assert!(rmat.is_ok());
assert_eq!(Robj::from(rmat), R!("matrix(1:12, nrow=4, byrow = TRUE)").unwrap());
}
}
Using the transpose crate would probably be way more efficient than what I did in the test above.
There was a problem hiding this comment.
I would say that the closure is very important. I can find 8 examples where I'm using it in existing code.
https://github.com/search?q=owner%3AJosiahParry%20new_matrix&type=code
It's very helpful in that I often iterate over rows and store a Vec<T> where T is some structure that is on a per-row basis. Presently I am writing code that creates a matrix from Vec<[f64;2]> with the closure |r, c| x[r][c].
There was a problem hiding this comment.
The closure is definitely useful, but I find it very confusing. That's why I removed it in the first place. The changes I made are very opinionated as I think these transformations made by the closure should be done outside extendr. When creating the array, I think the user should already have the Vec (or slice) already ready to be converted to an RArray.
There was a problem hiding this comment.
It requires a whole lot of mental gymnastics to understand how to create a flattened vector in the appropriate shape for R's column-major matrices. Not many people will be able to figure that out--myself included. QOL >>> imo
|
Hello @daniellga. I am honestly very sorry that we haven't resolved this. I want to solve it within the next two weeks. If you have availability to get it over the finish line, please tell me. |
|
@CGMossa and @JosiahParry I don't have time at all at the moment... Family stuff. Feel free to modify it if you think changes are worth. I think you are allowed to do it, right? (not very used to github) |
|
I still would like to improve this experience here. I continually find myself needing to make matrixes by row. Here is a current use case and makes it harder to achieve by removal of the closure approach which, I must admit is clunky, but better than nothing. Given a vector of a struct with a pub struct EsriCoord<const N: usize>(pub [f64; N]); |
I have been using
RArrayfor some time so I made changes that I think will make its use easier for an inexperienced user.So this PR is an
RArrayrebuild, making it easier to create anRArrayand also removingRColumnandRMatrix3D, which I don't think make that much sense.collect_rarraynow also accepts dynamic arguments likeVec.