Repository: rust-ndarray/ndarray (rust)

Issue:
Crash with par_apply_collect, debug, fortran order
I think I found a bug.

    use ndarray::{Array3, ShapeBuilder, Zip};

    fn main() {
        let data = Array3::<f32>::from_elem((5, 5, 5).f(), 1.0);
        println!("{}", Zip::from(&data).par_apply_collect(|d| d));
    }

It works flawlessly in `--release`, in 'c' order and with any size > 44. It crashes in 'f' order, debug and any array size from 1..45. The size part is really bizarre!

    thread 'thread 'thread '<unnamed><unnamed>thread '<unnamed>thread '' panicked at '<unnamed>thread '' panicked at '' panicked at '<unnamed>assertion failed: `(left == right)`
      left: `true`,
     right: `false`'
    ... lot of scrambled text ...

Tested on Windows, WSL and Ubuntu. Latest version of ndarray and Rust.

Pull request:
Fix bug in layout and broken debug assertion in collect to f-order array
This fixes a "bug" in layout_impl - if an array is effectively
one-dimensional, like for example has dimension 2 x 1 x 1 (3d array
with len two), then it's possible it is both C and F-contig.

This was the reason for the debug assertion firing: collection starts
into an f-order output array, but when we've split the array enough
times, it's still an f-order array but in shape 1 x 1 x 1 or 2 x 1 x 1
in this case - and we detected that as a c-order array.

1. fix layout calculation for this case
2. fix debug assertion condition for layout tendency

Fix #899

Patch:
diff --git a/src/layout/mod.rs b/src/layout/mod.rs
index 24dd0995..625fc7ff 100644
--- a/src/layout/mod.rs
+++ b/src/layout/mod.rs
@@ -78,6 +78,30 @@ mod tests {
     use crate::NdProducer;
 
     type M = Array2<f32>;
+    type M1 = Array1<f32>;
+    type M0 = Array0<f32>;
+
+    macro_rules! assert_layouts {
+        ($mat:expr, $($layout:expr),*) => {{
+            let layout = $mat.view().layout();
+            $(
+            assert!(layout.is($layout), "Assertion failed: array {:?} is not layout {}",
+                $mat,
+                stringify!($layout));
+            )*
+        }}
+    }
+
+    macro_rules! assert_not_layouts {
+        ($mat:expr, $($layout:expr),*) => {{
+            let layout = $mat.view().layout();
+            $(
+            assert!(!layout.is($layout), "Assertion failed: array {:?} show not have layout {}",
+                $mat,
+                stringify!($layout));
+            )*
+        }}
+    }
 
     #[test]
     fn contig_layouts() {
@@ -91,6 +115,34 @@ mod tests {
         assert!(af.is(FORDER) && af.is(FPREFER));
     }
 
+    #[test]
+    fn contig_cf_layouts() {
+        let a = M::zeros((5, 1));
+        let b = M::zeros((1, 5).f());
+        assert_layouts!(a, CORDER, CPREFER, FORDER, FPREFER);
+        assert_layouts!(b, CORDER, CPREFER, FORDER, FPREFER);
+
+        let a = M1::zeros(5);
+        let b = M1::zeros(5.f());
+        assert_layouts!(a, CORDER, CPREFER, FORDER, FPREFER);
+        assert_layouts!(b, CORDER, CPREFER, FORDER, FPREFER);
+
+        let a = M0::zeros(());
+        assert_layouts!(a, CORDER, CPREFER, FORDER, FPREFER);
+
+        let a = M::zeros((5, 5));
+        let b = M::zeros((5, 5).f());
+        let arow = a.slice(s![..1, ..]);
+        let bcol = b.slice(s![.., ..1]);
+        assert_layouts!(arow, CORDER, CPREFER, FORDER, FPREFER);
+        assert_layouts!(bcol, CORDER, CPREFER, FORDER, FPREFER);
+
+        let acol = a.slice(s![.., ..1]);
+        let brow = b.slice(s![..1, ..]);
+        assert_not_layouts!(acol, CORDER, CPREFER, FORDER, FPREFER);
+        assert_not_layouts!(brow, CORDER, CPREFER, FORDER, FPREFER);
+    }
+
     #[test]
     fn stride_layouts() {
         let a = M::zeros((5, 5));
diff --git a/src/zip/mod.rs b/src/zip/mod.rs
index dd0bce3f..4b233d09 100644
--- a/src/zip/mod.rs
+++ b/src/zip/mod.rs
@@ -58,7 +58,8 @@ where
     pub(crate) fn layout_impl(&self) -> Layout {
         let n = self.ndim();
         if self.is_standard_layout() {
-            if n <= 1 {
+            // effectively one-dimensional => C and F layout compatible
+            if n <= 1 || self.shape().iter().filter(|&&len| len > 1).count() <= 1 {
                 Layout::one_dimensional()
             } else {
                 Layout::c()
@@ -1173,8 +1174,19 @@ macro_rules! map_impl {
                 {
                     // Get the last producer; and make a Partial that aliases its data pointer
                     let (.., ref output) = &self.parts;
-                    debug_assert!(output.layout().is(CORDER | FORDER));
-                    debug_assert_eq!(output.layout().tendency() >= 0, self.layout_tendency >= 0);
+
+                    // debug assert that the output is contiguous in the memory layout we need
+                    if cfg!(debug_assertions) {
+                        let out_layout = output.layout();
+                        assert!(out_layout.is(CORDER | FORDER));
+                        assert!(
+                            (self.layout_tendency <= 0 && out_layout.tendency() <= 0) ||
+                            (self.layout_tendency >= 0 && out_layout.tendency() >= 0),
+                            "layout tendency violation for self layout {:?}, output layout {:?},\
+                            output shape {:?}",
+                            self.layout, out_layout, output.raw_dim());
+                    }
+
                     let mut partial = Partial::new(output.as_ptr());
 
                     // Apply the mapping function on this zip


Test patch:
diff --git a/tests/par_zip.rs b/tests/par_zip.rs
index f30d5c7e..01902911 100644
--- a/tests/par_zip.rs
+++ b/tests/par_zip.rs
@@ -107,20 +107,26 @@ fn test_zip_small_collect() {
 
     for m in 0..32 {
         for n in 0..16 {
-            let dim = (m, n);
-            let b = Array::from_shape_fn(dim, |(i, j)| 1. / (i + 2 * j + 1) as f32);
-            let c = Array::from_shape_fn(dim, |(i, j)| f32::ln((1 + i + j) as f32));
-
-            {
-                let a = Zip::from(&b).and(&c).par_map_collect(|x, y| x + y);
-
-                assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6);
-                assert_eq!(a.strides(), b.strides());
+            for &is_f in &[false, true] {
+                let dim = (m, n).set_f(is_f);
+                let b = Array::from_shape_fn(dim, |(i, j)| 1. / (i + 2 * j + 1) as f32);
+                let c = Array::from_shape_fn(dim, |(i, j)| f32::ln((1 + i + j) as f32));
+
+                {
+                    let a = Zip::from(&b).and(&c).par_map_collect(|x, y| x + y);
+
+                    assert_abs_diff_eq!(a, &b + &c, epsilon = 1e-6);
+                    if m > 1 && n > 1 {
+                        assert_eq!(a.strides(), b.strides(),
+                            "Failure for {}x{} c/f: {:?}", m, n, is_f);
+                    }
+                }
             }
         }
     }
 }
 
+
 #[test]
 #[cfg(feature = "approx")]
 fn test_zip_assign_into() {