Stack and concatenate #735

andrei-papou · 2019-10-01T13:05:43Z

Issue: #667

Renamed stack to concatenate to follow the behaviour of numpy and also added stack function. Examples:

concatenate

use ndarray::{arr2, Axis, concatenate};

let a = arr2(&[[2., 2.], [3., 3.]]);
assert!(
     concatenate(Axis(0), &[a.view(), a.view()])
     == Ok(arr2(&[[2., 2.], [3., 3.], [2., 2.], [3., 3.]]))
);

stack

use ndarray::{arr2, arr3, stack, Axis};

let a = arr2(&[[2., 2.],
               [3., 3.]]);
assert!(
    stack![Axis(0), a, a]
    == arr3(&[[[2., 2.],
               [3., 3.]],
              [[2., 2.],
               [3., 3.]]])
);

Merge main rep master to fork master

Update fork master

Update master

Up to date with the main repo master

bluss · 2019-10-01T13:36:06Z

Nice, this is the right direction. I think it would be best to make this in the shape of a non breaking change to start with so that it can be included soon.

bluss · 2019-10-02T05:45:53Z

src/stacking.rs

+    }
+    let arrays: Vec<ArrayView<A, D::Larger>> =
+        arrays.into_iter().map(|a| a.insert_axis(axis)).collect();
+    concatenate(axis, &arrays)


Implementation strategy wise, can't we take the old stack, put that into an internal function. Give it a parameter that tells it whether to remove an axis or not. It can be a type parameter, since it influences the return type (the dim of the returned array).

That way both stack and concatenate can use this internal function and we don't need the intermediate vector here in concatenate

Looks good! I'll try to implement it this way.

bluss · 2019-10-02T07:18:38Z

This branch will eventually need to be rebased to skip the merges in the history (see commit view). The blast back to #1, #5, #8 etc in ndarray history is fun, but we don't want to have those merges there.

Also, "this is the right direction" isn't supposed to sound like "nice try". It's supposed to say.. This is the right destination, this is how we want to name those functions. To be pragmatic I think we'd like to have this in 0.13.x, as a non-breaking change in the first round (preparing to put the functions in their right places in the next version).

function

- got rid of the redundant allocation in `stack_new_axis` implementation - renamed the functions and macros to make the change backward compatible: `concatenate` -> `stack`, `stack` -> `stack_new_axis`

Merge main repo master to the fork

andrei-papou · 2019-10-17T00:14:45Z

@bluss sorry for the delay, the month is pretty busy. I've refactored the new function to get rid of the redundant allocation. Also I renamed the functions in the backward compatible way. Thanks.

LukeMathWalker · 2019-12-15T19:37:09Z

src/stacking.rs

 ///
 /// Uses the [`stack`][1] function, calling `ArrayView::from(&a)` on each
 /// argument `a`.
 ///
 /// [1]: fn.stack.html
 ///
-/// ***Panics*** if the `stack` function would return an error.
+/// ***Panics*** if the `concatenate` function would return an error.


concatenate does not exist yet, we shouldn't refer to it here.

LukeMathWalker · 2019-12-15T19:41:18Z

If we want to introduce this in 0.13.x, we should probably:

mark the existing stack function as deprecated, suggesting to use stack_new_axis;
introduce concatenate and concatenate!, which underneath are just calling stack for now;
introduce stack_new_axis which will become the "new" stack.

What do you think? @andrei-papou
Always difficult to change what a name means :/

andrei-papou · 2019-12-16T14:25:29Z

mark the existing stack function as deprecated, suggesting to use stack_new_axis;

@LukeMathWalker shouldn't we suggest to use concatenate instead? concatenate is a new name for the current stack function while stack_new_axis behaves in a different way.

LukeMathWalker · 2019-12-16T14:27:56Z

Yes, you are right 😅

- introduced concatenate function - deprecated stack function since 0.13.0

- updated deprecation version for stack function - suppressed deprecation warnings

Main repo master to fork master

…nd-concatenate

andrei-papou · 2019-12-18T19:46:23Z

@LukeMathWalker ready, but I had to suppress a few deprecation warnings: we still need stack function in concatenate implementation, in lib.rs and in tests.

andrei-papou · 2020-03-30T07:49:15Z

@bluss @LukeMathWalker could you please do another round of review? Looks like all the comments have been resolved quite a while ago. Thanks.

bluss · 2020-04-18T11:16:18Z

src/stacking.rs

+
+pub fn stack_new_axis<A, D>(
+    axis: Axis,
+    arrays: Vec<ArrayView<A, D>>,


stack_new_axis and concatenate should agree on if they take the arrays as a slice or a Vec, they should have the same interface if they can. Avoiding cloning of the array views seems like a minor issue, so it can be decided either way (slice or vec)

bluss · 2020-04-18T11:16:40Z

src/stacking.rs

+    stack(axis, arrays)
+}
+
+pub fn stack_new_axis<A, D>(


Missing doc comment :)

bluss · 2020-04-18T11:17:06Z

src/stacking.rs

@@ -109,3 +182,71 @@ macro_rules! stack {
        $crate::stack($axis, &[ $($crate::ArrayView::from(&$array) ),* ]).unwrap()


The stack macro should also be deprecated

bluss · 2020-04-18T11:18:10Z

src/stacking.rs

+    res_dim.set_axis(axis, arrays.len());
+
+    // we can safely use uninitialized values here because they are Copy
+    // and we will only ever write to them


This looks good, it follows what we do in stack. We'll revise handling of uninit data, but it's not in scope for this PR. (#796 )

bluss · 2020-04-18T11:19:18Z

src/stacking.rs

+    let mut res = Array::from_shape_vec(res_dim, v)?;
+
+    res.axis_iter_mut(axis)
+        .zip(arrays.into_iter())


this zip seems to show that we would be fine with just using a slice of ArrayView as input, we don't need to use them by value

bluss · 2020-04-20T19:23:57Z

src/slice.rs

@@ -544,62 +544,62 @@ impl_slicenextdim_larger!((), Slice);
 #[macro_export]
 macro_rules! s(
    // convert a..b;c into @convert(a..b, c), final item
-    (@parse $dim:expr, [$($stack:tt)*] $r:expr;$s:expr) => {
+    (@parse $dim:expr, [$($concatenate:tt)*] $r:expr;$s:expr) => {


This rename of the variable inside the macro is an incidental mistake, and should not be in the PR

Oh I removed this in my rebase-fix of the branch. No other changes in that branch. :)

bluss · 2020-04-21T14:30:52Z

I have rebased your commit history and pushed to a new branch: See https://github.com/rust-ndarray/ndarray/pull/new/ap/stack-and-concatenate

Please use it as a starting point - it removes all the merges in the commit history of this PR. We accept some unclean histories, but this needed to be cleaned up before merging.

There are some commits that have the title "Changelog:" please edit those, and make sure the first line is a summary of what the commit does. See my commits in the repo, if you want an example. Thanks :)

Andrew and others added 4 commits April 3, 2019 11:54

Merge pull request #1 from rust-ndarray/master

b39a0bb

Merge main rep master to fork master

Merge pull request #5 from rust-ndarray/master

efd686d

Update fork master

Merge pull request #8 from rust-ndarray/master

8cd5d5a

Update master

Merge pull request #9 from rust-ndarray/master

0e9d65d

Up to date with the main repo master

bluss reviewed Oct 2, 2019

View reviewed changes

andrei-papou added 5 commits October 2, 2019 11:02

Renamed stack to concatenate and added numpy-compliant stack

6525f6e

function

Rustfmt fixes

2d04320

Commit changelog:

ced7ed9

- got rid of the redundant allocation in `stack_new_axis` implementation - renamed the functions and macros to make the change backward compatible: `concatenate` -> `stack`, `stack` -> `stack_new_axis`

Merge pull request #10 from rust-ndarray/master

443fa86

Merge main repo master to the fork

Rustfmt fixes

68f0e95

andrei-papou mentioned this pull request Dec 13, 2019

Add dstack, vstack, and hstack #667

Closed

LukeMathWalker reviewed Dec 15, 2019

View reviewed changes

andrei.papou added 4 commits December 18, 2019 21:54

Changelog:

d997c18

- introduced concatenate function - deprecated stack function since 0.13.0

Changelog:

8e12f82

- updated deprecation version for stack function - suppressed deprecation warnings

Merge pull request #11 from rust-ndarray/master

ef3d5b6

Main repo master to fork master

Merge branch 'master' of github.com:andrei-papou/ndarray into stack-a…

1e91fe4

…nd-concatenate

bluss self-assigned this Apr 16, 2020

bluss mentioned this pull request Apr 18, 2020

Support Clone elements in stack and select #269

Closed

bluss reviewed Apr 18, 2020

View reviewed changes

src/stacking.rs

stack(axis, arrays)

}

pub fn stack_new_axis<A, D>(

Copy link

Member

bluss Apr 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing doc comment :)

bluss reviewed Apr 18, 2020

View reviewed changes

bluss reviewed Apr 20, 2020

View reviewed changes

andrei-papou mentioned this pull request Sep 25, 2020

Stack and concatenate #844

Merged

andrei-papou closed this Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stack and concatenate #735

Stack and concatenate #735

andrei-papou commented Oct 1, 2019 •

edited

Loading

bluss commented Oct 1, 2019

bluss Oct 2, 2019

andrei-papou Oct 2, 2019

bluss commented Oct 2, 2019 •

edited

Loading

andrei-papou commented Oct 17, 2019

LukeMathWalker Dec 15, 2019

LukeMathWalker commented Dec 15, 2019

andrei-papou commented Dec 16, 2019

LukeMathWalker commented Dec 16, 2019

andrei-papou commented Dec 18, 2019

andrei-papou commented Mar 30, 2020

bluss Apr 18, 2020 •

edited

Loading

bluss Apr 18, 2020

bluss Apr 18, 2020

bluss Apr 18, 2020

bluss Apr 18, 2020

bluss Apr 20, 2020

bluss Apr 21, 2020

bluss commented Apr 21, 2020

		@@ -109,3 +182,71 @@ macro_rules! stack {
		$crate::stack($axis, &[ $($crate::ArrayView::from(&$array) ),* ]).unwrap()

Stack and concatenate #735

Stack and concatenate #735

Conversation

andrei-papou commented Oct 1, 2019 • edited Loading

concatenate

stack

bluss commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Oct 2, 2019 • edited Loading

andrei-papou commented Oct 17, 2019

Choose a reason for hiding this comment

LukeMathWalker commented Dec 15, 2019

andrei-papou commented Dec 16, 2019

LukeMathWalker commented Dec 16, 2019

andrei-papou commented Dec 18, 2019

andrei-papou commented Mar 30, 2020

bluss Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Apr 21, 2020

andrei-papou commented Oct 1, 2019 •

edited

Loading

bluss commented Oct 2, 2019 •

edited

Loading

bluss Apr 18, 2020 •

edited

Loading