Skip to content
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

Document how to handle matrix #76

Closed
yutannihilation opened this issue Jan 26, 2024 · 15 comments
Closed

Document how to handle matrix #76

yutannihilation opened this issue Jan 26, 2024 · 15 comments
Labels
documentation Improvements or additions to documentation

Comments

@yutannihilation
Copy link
Owner

No description provided.

@yutannihilation yutannihilation added the documentation Improvements or additions to documentation label Jan 26, 2024
@daniellga
Copy link
Contributor

Could you make it more generic and try to document the array case instead?
I don't have time at all right now but will soon try to port my package to use savvy instead of extendr. The simpler API really sold me.

@yutannihilation
Copy link
Owner Author

yutannihilation commented Feb 4, 2024

Oh, thanks for your interest!

You can use get_dim() to get the dimensions and re-arrange the underlying vector from column-major order to row-major order. Since this is a common problem when you deal with R and Python, I think you can come up with an implementation that just works.

cf. https://cran.r-project.org/web/packages/reticulate/vignettes/arrays.html

What I think I'll have to struggle about is how to do it efficiently. Since I'm not familiar with matrix and array, it might take some time.

By the way, just curious, I think extendr is simple enough for simple usages. In what part of extendr do you feel is not simple?

@daniellga
Copy link
Contributor

There's a couple of months I don't use extendr but here is what I can remember:

  • Implicit conversion, which makes everything more confusing IMO. I like to have control on these conversions, so I have to carefully write them on my own.
  • Conversion in general can be confusing with the existence of the FromRobj trait in addition to From and TryFrom.
  • Error handling.
  • scalar vs vector differentiation, for example Rint and Integers, can be confusing to new users and make the crate more complex.

For error handling, I dont think this crate solves my problem of using my own rust errors in R. I have an old pIan of using external pointers of an enum to represent my own errors on rust side and unwrap these it on the R side, but the lack of ? (Try trait) makes it harder to implemement, so I am waiting the rust team to stabilize it.

@yutannihilation
Copy link
Owner Author

yutannihilation commented Feb 4, 2024

Thanks for sharing, I see. I agree, if you prefer explicitness, probably my framework should fit better than extendr.

I added a simple example of using array with conversion from R to Rust (ndarray). Does this help?

https://github.com/yutannihilation/savvy/blob/734453c9e6bbf5a2556e992ca60f98f26d87cf3a/R-package/src/rust/src/array.rs

@daniellga
Copy link
Contributor

Nice! I think an example helps a lot new users, the only missing thing is the reverse conversion, from ndarray to an R array, probably using set_attrib.

@yutannihilation
Copy link
Owner Author

Yes, I need a reverse conversion example... You can use set_dim() for setting the dim attribute.

https://yutannihilation.github.io/savvy/savvy/sexp/real/struct.OwnedRealSexp.html#method.set_dim

@yutannihilation
Copy link
Owner Author

yutannihilation commented Feb 4, 2024

Note to self. It seems I was wrong here.

re-arrange the underlying vector from column-major order to row-major order.

  • ndarray: row-major is default (probably for compatibility with Python ndarray?), but it offers column-major as well
  • nalgebra: column-major
  • glam (and probably all other rust-gamedev crates): column-major, probably because GLSL is column-major

@yutannihilation
Copy link
Owner Author

@daniellga
I wrote some toy examples of matirx usages including ndarray! I'd really appreciate your feedback because I'm not familiar with matrix things at all. When you have time, could you take a look?

https://github.com/yutannihilation/savvy-matrix-examples/tree/master/src/rust/src

@daniellga
Copy link
Contributor

Hey @yutannihilation! Sorry for taking so long, I am really struggling to find some time to code. I looked at your example code for ndarray and have some points:

  • Shouldn't get_dim() return a read only slice (or an IntegerSexp) instead of a Vec, avoiding a new allocation? Then the user could create a Vec from it if needed.

  • Maybe something like the code below is simpler for ndarray_output:

    /// @export
    #[savvy]
    fn ndarray_output() -> savvy::Result<savvy::Sexp> {
        // By default, memory layout is row major.
        let a_row_major = array![[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]];
        let a_column_major = a_row_major.reversed_axes();
        let shape = a_column_major.shape();
    
        let mut out = OwnedRealSexp::try_from(a_column_major.as_slice_memory_order().ok_or("data must be contiguous")?)?;
        out.set_dim(shape)?; 
    
        out.into()
    }
    

    or just messing with the shape, though I have no idea if it is more costly (I think so):

    /// @export
    #[savvy]
    fn ndarray_output() -> savvy::Result<savvy::Sexp> {
        // By default, memory layout is row major.
        let a_row_major = array![[1.0, 2.0], [3.0, 4.0], [5.0, 6.0]];
        //extra allocation here.
        let shape = a_row_major.shape().iter().rev().cloned().collect::<Vec<usize>>(); 
    
        let mut out = OwnedRealSexp::try_from(a_row_major.as_slice().ok_or("data must be contiguous and in C order")?)?;
        out.set_dim(shape.as_slice())?; 
    
        out.into()
    }
    

I run into weird errors when doing cargo test, is this expected?
I couldn't test this suggestion since I have no R on the PC I am using right now. It compiles though...

@yutannihilation
Copy link
Owner Author

Oh, thanks for the suggestions!

I run into weird errors when doing cargo test, is this expected?

Yes. savvy doesn't work without a real R session. While you might feel comfortable with extendr's experience of cargo test, it causes a lot of false errors that never happens on a real R session, and it requires many complex tweaks. See extendr/extendr#583 for more details.

I'm hoping savvy can eventually work with cargo test, but it's not today. Here's some ideas:

@yutannihilation
Copy link
Owner Author

  • Shouldn't get_dim() return a read only slice (or an IntegerSexp) instead of a Vec, avoiding a new allocation? Then the user could create a Vec from it if needed.

While I thought this is a good idea, now I think this isn't 100% safe. As long as the SEXP of dim is tied to the matrix, it's protected. However, if a new dim is set, it can get GC-ed.

@daniellga
Copy link
Contributor

But what if I get dim by using get_attrib, would that be unsafe as well?

@yutannihilation
Copy link
Owner Author

Ah, sorry, please forget my comment above. The input SEXP is read-only, so dim is never overwritten within the Rust function.

@yutannihilation
Copy link
Owner Author

Hmm, my Rust-fu is not enough to pass the lifetime check. I'm giving up for now. Sorry.

@daniellga
Copy link
Contributor

No problem! A way easier solution seems to be returning the IntegerSexp in get_dim_from_sexp instead of a slice but that's up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants