Repository: extendr/extendr
Pull request #450 (2023-01-17 14:12:39)

Document the robj_ndarray module
* Document the robj_ndarray module 
* Add conversion for owned arrays, with tests

multimeric: Two questions for whoever reviews this:

1. Why is the WIndows build failing? Is that at all relating to my PR?
2. Why does it seem like the doctests aren't being run for this PR. I've added a number of examples that do run correctly and it would be nice to know that they're being checked.
constantingoeldel: Thanks for the documentation, this already helped me!
multimeric: Going to hold off on merging this until we merge #460, so I can rewrite to use `rstest`.
multimeric: Has now been updated to use `rstest`. Once the CI is running again, this is good to go.

Patch:
diff --git a/extendr-api/src/robj/into_robj.rs b/extendr-api/src/robj/into_robj.rs
index fda3b24de..8f8ce8ce4 100644
--- a/extendr-api/src/robj/into_robj.rs
+++ b/extendr-api/src/robj/into_robj.rs
@@ -477,6 +477,43 @@ pub trait RobjItertools: Iterator {
         assert!(vec.iter().size_hint() == (vec.len(), Some(vec.len())));
         vec.into_iter().collect_robj()
     }
+
+    /// Collects an iterable into an [`RArray`].
+    /// The iterable must yield items column by column (aka Fortan order)
+    ///
+    /// # Arguments
+    ///
+    /// * `dims` - an array containing the length of each dimension
+    fn collect_rarray<'a, const LEN: usize>(
+        self,
+        dims: [usize; LEN],
+    ) -> Result<RArray<Self::Item, [usize; LEN]>>
+    where
+        Self: Iterator,
+        Self: Sized,
+        Self::Item: ToVectorValue,
+        Robj: AsTypedSlice<'a, Self::Item>,
+        Self::Item: 'a,
+    {
+        let vector = self.collect_robj();
+        let prod = dims.iter().product::<usize>();
+        if prod != vector.len() {
+            return Err(Error::Other(format!(
+                "The vector length ({}) does not match the length implied by the dimensions ({})",
+                vector.len(),
+                prod
+            )));
+        }
+        let mut robj =
+            vector.set_attrib(wrapper::symbol::dim_symbol(), dims.iter().collect_robj())?;
+        let data = robj
+            .as_typed_slice_mut()
+            .ok_or(Error::Other(
+                "Unknown error in converting to slice".to_string(),
+            ))?
+            .as_mut_ptr();
+        Ok(RArray::from_parts(robj, data, dims))
+    }
 }
 
 // Thanks to *pretzelhammer* on stackoverflow for this.
@@ -605,3 +642,52 @@ impl From<Vec<Rfloat>> for Robj {
         Doubles::from_values(val.into_iter()).into()
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_collect_rarray_matrix() {
+        test! {
+            // Check that collect_rarray works the same as R's matrix() function
+            let rmat = (1i32..=16).collect_rarray([4, 4]);
+            assert!(rmat.is_ok());
+            assert_eq!(Robj::from(rmat), R!("matrix(1:16, nrow=4)").unwrap());
+        }
+    }
+
+    #[test]
+    fn test_collect_rarray_tensor() {
+        test! {
+            // Check that collect_rarray works the same as R's array() function
+            let rmat = (1i32..=16).collect_rarray([2, 4, 2]);
+            assert!(rmat.is_ok());
+            assert_eq!(Robj::from(rmat), R!("array(1:16, dim=c(2, 4, 2))").unwrap());
+        }
+    }
+
+    #[test]
+    fn test_collect_rarray_matrix_failure() {
+        test! {
+            // Check that collect_rarray fails when given an invalid shape
+            let rmat = (1i32..=16).collect_rarray([3, 3]);
+            assert!(rmat.is_err());
+            let msg = rmat.unwrap_err().to_string();
+            assert!(msg.contains("9"));
+            assert!(msg.contains("dimension"));
+        }
+    }
+
+    #[test]
+    fn test_collect_tensor_failure() {
+        test! {
+            // Check that collect_rarray fails when given an invalid shape
+            let rmat = (1i32..=16).collect_rarray([3, 3, 3]);
+            assert!(rmat.is_err());
+            let msg = rmat.unwrap_err().to_string();
+            assert!(msg.contains("27"));
+            assert!(msg.contains("dimension"));
+        }
+    }
+}
diff --git a/extendr-api/src/robj/mod.rs b/extendr-api/src/robj/mod.rs
index 2b3cb94cd..ca8dc0c0f 100644
--- a/extendr-api/src/robj/mod.rs
+++ b/extendr-api/src/robj/mod.rs
@@ -1,11 +1,11 @@
 //! R object handling.
 //!
-//! See. <https://cran.r-project.org/doc/manuals/R-exts.html>
+//! See. [Writing R Extensions](https://cran.r-project.org/doc/manuals/R-exts.html)
 //!
 //! Fundamental principals:
 //!
 //! * Any function that can break the protection mechanism is unsafe.
-//! * Users should be able to do almost everything without using libR_sys.
+//! * Users should be able to do almost everything without using `libR_sys`.
 //! * The interface should be friendly to R users without Rust experience.
 //!
 
diff --git a/extendr-api/src/robj_ndarray.rs b/extendr-api/src/robj_ndarray.rs
index 8ea4aedcb..ef9daefb6 100644
--- a/extendr-api/src/robj_ndarray.rs
+++ b/extendr-api/src/robj_ndarray.rs
@@ -1,3 +1,63 @@
+/*!
+Defines conversions between R objects and the [ndarray](https://docs.rs/ndarray/latest/ndarray/) crate, which offers native Rust array types and numerical computation routines.
+
+To enable these conversions, you must first enable the `ndarray` feature for extendr:
+```toml
+[dependencies]
+extendr-api = { version = "0.3.1", features = ["ndarray"] }
+```
+
+Specifically, extendr supports the following conversions:
+* [`Robj` → `ArrayView1`](FromRobj#impl-FromRobj<%27a>-for-ArrayView1<%27a%2C%20T>), for when you have an R vector that you want to analyse in Rust:
+    ```rust
+    use extendr_api::prelude::*;
+    use ndarray::ArrayView1;
+
+    #[extendr]
+    fn describe_vector(vector: ArrayView1<f64>){
+        println!("This R vector has length {:?}", vector.len())
+    }
+    ```
+* [`Robj` → `ArrayView2`](FromRobj#impl-FromRobj<%27a>-for-ArrayView2<%27a%2C%20f64>), for when you have an R matrix that you want to analyse in Rust.
+    ```rust
+    use extendr_api::prelude::*;
+    use ndarray::ArrayView2;
+
+    #[extendr]
+    fn describe_matrix(matrix: ArrayView2<f64>){
+        println!("This R matrix has shape {:?}", matrix.dim())
+    }
+    ```
+* [`ArrayBase` → `Robj`](Robj#impl-TryFrom<ArrayBase<S%2C%20D>>-for-Robj), for when you want to return a reference to an [`ndarray`] Array from Rust back to R.
+    ```rust
+    use extendr_api::prelude::*;
+    use ndarray::Array2;
+
+    #[extendr]
+    fn return_matrix() -> Robj {
+        Array2::<f64>::zeros((4, 4)).try_into().unwrap()
+    }
+    ```
+
+The item type (ie the `T` in [`Array2<T>`]) can be a variety of Rust types that can represent scalars: [`u32`], [`i32`], [`f64`] and, if you have the `num_complex` compiled feature
+enabled, `Complex<f64>`. Items can also be extendr's wrapper types: [`Rbool`], [`Rint`], [`Rfloat`] and [`Rcplx`].
+
+Note that the extendr-ndarray integration only supports accessing R arrays as [`ArrayView`], which are immutable.
+Therefore, instead of directly editing the input array, it is recommended that you instead return a new array from your `#[extendr]`-annotated function, which you allocate in Rust.
+It will then be copied into a new block of memory managed by R.
+This is made easier by the fact that [ndarray allocates a new array automatically when performing operations on array references](ArrayBase#binary-operators-with-array-and-scalar):
+```rust
+use extendr_api::prelude::*;
+use ndarray::Array2;
+
+#[extendr]
+fn scalar_multiplication(matrix: ArrayView2<f64>, scalar: f64) -> Robj {
+    (&matrix * scalar).try_into().unwrap()
+}
+```
+
+For all array uses in Rust, refer to the [`ndarray::ArrayBase`] documentation, which explains the usage for all of the above types.
+*/
 #[doc(hidden)]
 use ndarray::prelude::*;
 use ndarray::{Data, ShapeBuilder};
@@ -113,6 +173,8 @@ where
 {
     type Error = Error;
 
+    /// Converts a reference to an ndarray Array into an equivalent R array.
+    /// The data itself is copied.
     fn try_from(value: &ArrayBase<S, D>) -> Result<Self> {
         // Refer to https://github.com/rust-ndarray/ndarray/issues/1060 for an excellent discussion
         // on how to convert from `ndarray` types to R/fortran arrays
@@ -142,6 +204,21 @@ where
     }
 }
 
+impl<A, S, D> TryFrom<ArrayBase<S, D>> for Robj
+where
+    S: Data<Elem = A>,
+    A: Copy + ToVectorValue,
+    D: Dimension,
+{
+    type Error = Error;
+
+    /// Converts an ndarray Array into an equivalent R array.
+    /// The data itself is copied.
+    fn try_from(value: ArrayBase<S, D>) -> Result<Self> {
+        Robj::try_from(&value)
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::*;
@@ -201,45 +278,55 @@ mod test {
     #[rstest]
     #[case(
         // An empty array should still convert to an empty R array with the same shape
-        &Array4::<i32>::zeros((0, 1, 2, 3).f()),
+        Array4::<i32>::zeros((0, 1, 2, 3).f()),
         "array(integer(), c(0, 1, 2, 3))"
     )]
     #[case(
-        &array![1., 2., 3.],
+        array![1., 2., 3.],
         "array(c(1, 2, 3))"
     )]
     #[case(
         // We give both R and Rust the same 1d vector and tell them both to read it as a matrix in C order.
         // Therefore these arrays should be the same.
-        &Array::from_shape_vec((2, 3), vec![1., 2., 3., 4., 5., 6.]).unwrap(),
+        Array::from_shape_vec((2, 3), vec![1., 2., 3., 4., 5., 6.]).unwrap(),
         "matrix(c(1, 2, 3, 4, 5, 6), nrow=2, byrow=TRUE)"
     )]
     #[case(
         // We give both R and Rust the same 1d vector and tell them both to read it as a matrix
         // in fortran order. Therefore these arrays should be the same.
-        &Array::from_shape_vec((2, 3).f(), vec![1., 2., 3., 4., 5., 6.]).unwrap(),
+        Array::from_shape_vec((2, 3).f(), vec![1., 2., 3., 4., 5., 6.]).unwrap(),
         "matrix(c(1, 2, 3, 4, 5, 6), nrow=2, byrow=FALSE)"
     )]
     #[case(
         // We give both R and Rust the same 1d vector and tell them both to read it as a 3d array
         // in fortran order. Therefore these arrays should be the same.
-        &Array::from_shape_vec((1, 2, 3).f(), vec![1, 2, 3, 4, 5, 6]).unwrap(),
+        Array::from_shape_vec((1, 2, 3).f(), vec![1, 2, 3, 4, 5, 6]).unwrap(),
         "array(1:6, c(1, 2, 3))"
     )]
     #[case(
         // We give R a 1d vector and tell it to read it as a 3d vector
         // Then we give Rust the equivalent vector manually split out.
-        &array![[[1, 5], [3, 7]], [[2, 6], [4, 8]]],
+        array![[[1, 5], [3, 7]], [[2, 6], [4, 8]]],
         "array(1:8, dim=c(2, 2, 2))"
     )]
-    fn test_to_robj<T>(#[case] array: T, #[case] r_expr: &str)
-    where
-        Robj: TryFrom<T>,
-        <Robj as TryFrom<T>>::Error: std::fmt::Debug,
+    fn test_to_robj<ElementType, DimType>(
+        #[case] array: Array<ElementType, DimType>,
+        #[case] r_expr: &str,
+    ) where
+        Robj: TryFrom<Array<ElementType, DimType>>,
+        for<'a> Robj: TryFrom<&'a Array<ElementType, DimType>>,
+        <robj::Robj as TryFrom<Array<ElementType, DimType>>>::Error: std::fmt::Debug,
+        for<'a> <robj::Robj as TryFrom<&'a Array<ElementType, DimType>>>::Error: std::fmt::Debug,
     {
         // Tests for the Rust → R conversion, so we therefore perform the
         // comparison in R
         test! {
+            // Test for borrowed array
+            assert_eq!(
+                &(Robj::try_from(&array).unwrap()),
+                &eval_string(r_expr).unwrap()
+            );
+            // Test for owned array
             assert_eq!(
                 &(Robj::try_from(array).unwrap()),
                 &eval_string(r_expr).unwrap()
diff --git a/extendr-api/src/wrapper/matrix.rs b/extendr-api/src/wrapper/matrix.rs
index 9dd20c8d2..50894e73e 100644
--- a/extendr-api/src/wrapper/matrix.rs
+++ b/extendr-api/src/wrapper/matrix.rs
@@ -126,7 +126,16 @@ where
     Robj: AsTypedSlice<'a, T>,
 {
     /// Create a new matrix wrapper.
-    /// Make a new column type.
+    ///
+    /// # Arguments
+    ///
+    /// * `nrows` - the number of rows the returned matrix will have
+    /// * `ncols` - the number of columns the returned matrix will have
+    /// * `f` - a function that will be called for each entry of the matrix in order to populate it with values.
+    ///     It must return a scalar value that can be converted to an R scalar, such as `u32`, `f64`, i.e. see [ToVectorValue].
+    ///     It accepts two arguments:
+    ///     * `r` - the current row of the entry we are creating
+    ///     * `c` - the current column of the entry we are creating
     pub fn new_matrix<F: Clone + FnMut(usize, usize) -> T>(
         nrows: usize,
         ncols: usize,
diff --git a/extendr-engine/src/lib.rs b/extendr-engine/src/lib.rs
index 72562ee4d..19dead126 100644
--- a/extendr-engine/src/lib.rs
+++ b/extendr-engine/src/lib.rs
@@ -1,17 +1,18 @@
-//! A sigleton instance of the R interpreter.
+//! A singleton instance of the R interpreter.
 //!
-//! Only call this from main() if you want to run stand-alone.
+//! Only call this from `main()` if you want to run stand-alone.
 //!
 //! Its principal use is for testing.
 //!
-//! See <https://github.com/wch/r-source/blob/trunk/src/unix/Rembedded.c>
+//! See [Rembedded.c](https://github.com/wch/r-source/blob/trunk/src/unix/Rembedded.c).
+//!
 
 use libR_sys::*;
 use std::os::raw;
 use std::sync::Once;
 
 // Generate mutable static strings.
-// Much more efficient than CString.
+// Much more efficient than `CString`.
 // Generates asciiz.
 macro_rules! cstr_mut {
     ($s: expr) => {
diff --git a/extendr-macros/src/extendr_module.rs b/extendr-macros/src/extendr_module.rs
index 688b1c29f..d6f003454 100644
--- a/extendr-macros/src/extendr_module.rs
+++ b/extendr-macros/src/extendr_module.rs
@@ -48,8 +48,8 @@ pub fn extendr_module(item: TokenStream) -> TokenStream {
             #( #implmetanames(&mut impls); )*
 
             // Extends functions and impls with the submodules metadata
-            #( functions.extend(#usemetanames().functions); )*
-            #( impls.extend(#usemetanames().impls); )*
+            #( functions.extend(#usenames::#usemetanames().functions); )*
+            #( impls.extend(#usenames::#usemetanames().impls); )*
 
             // Add this function to the list, but set hidden: true.
             functions.push(extendr_api::metadata::Func {
@@ -88,6 +88,7 @@ pub fn extendr_module(item: TokenStream) -> TokenStream {
         #[no_mangle]
         #[allow(non_snake_case)]
         pub extern "C" fn #wrap_module_metadata_name() -> extendr_api::SEXP {
+            use extendr_api::GetSexp;
             unsafe { extendr_api::Robj::from(#module_metadata_name()).get() }
         }
 
@@ -99,6 +100,7 @@ pub fn extendr_module(item: TokenStream) -> TokenStream {
         ) -> extendr_api::SEXP {
             unsafe {
                 use extendr_api::robj::*;
+                use extendr_api::GetSexp;
                 let robj = new_owned(use_symbols_sexp);
                 let use_symbols: bool = <bool>::from_robj(&robj).unwrap();
 


Test patch:
diff --git a/extendr-api/tests/extendr_module_tests.rs b/extendr-api/tests/extendr_module_tests.rs
new file mode 100644
index 000000000..3aeea81ba
--- /dev/null
+++ b/extendr-api/tests/extendr_module_tests.rs
@@ -0,0 +1,41 @@
+//! Tests for [`extendr_module`] procedural macro.
+//!
+use extendr_api::{extendr, extendr_module};
+
+mod root {
+    use super::*;
+
+    mod nested_module {
+        use super::*;
+
+        #[extendr]
+        fn dummy() {}
+
+        extendr_module! {
+            mod nested_module;
+            fn dummy;
+        }
+    }
+
+    #[extendr]
+    fn hello_dummy() {}
+
+    extendr_module! {
+        mod top_level;
+        use nested_module;
+        use adjacent_module;
+        fn hello_dummy;
+    }
+}
+
+mod adjacent_module {
+    use super::*;
+
+    #[extendr]
+    fn foo() {}
+
+    extendr_module! {
+        mod adjacent_module;
+        fn foo;
+    }
+}
diff --git a/tests/extendrtests/src/rust/src/lib.rs b/tests/extendrtests/src/rust/src/lib.rs
index 94a8072db..bccc77b91 100644
--- a/tests/extendrtests/src/rust/src/lib.rs
+++ b/tests/extendrtests/src/rust/src/lib.rs
@@ -1,7 +1,6 @@
 use extendr_api::{graphics::*, prelude::*};
 
 mod submodule;
-use submodule::*;
 
 mod ndarray;
 use ndarray::*;
@@ -170,7 +169,7 @@ fn check_default(#[default = "NULL"] x: Robj) -> bool {
 }
 
 // Weird behavior of parameter descriptions:
-// first passes tests as is, second -- only in backqutoes.
+// first passes tests as is, second -- only in backquotes.
 /// Test whether `_arg` parameters are treated correctly in R
 /// Executes \code{`_x` - `_y`}
 /// @param _x an integer scalar, ignored