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

Add shuffle_axis_inplace and shuffle_axis_inplace_using #742

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LukeMathWalker
Copy link
Member

Other two simple functions that I think might useful when working with arrays.

Should we add also sampling_axis and sampling_axis_using as convenient shims for

let a = self.clone();
a.shuffle_axis_inplace(axis);
a

?

let slice1 = self.index_axis(axis, i);
let slice2 = self.index_axis(axis, j);

for (x, y) in slice1.iter().zip(slice2.iter()) {
Copy link
Member

@bluss bluss Oct 9, 2019

Choose a reason for hiding this comment

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

We are modifying through read-only/shared array views and this is invalid, unless we have interior mutability (i.e using Cell, Mutex or similar).

And we'd like to avoid iter().zip() for ndarray, because it is much slower than it needs to be, we have Zip.

Copy link
Member

@bluss bluss Oct 9, 2019

Choose a reason for hiding this comment

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

I'm working on making ArrayView<Cell<T>, D> a reality, a conversion we can use sometimes, - it allows us to have.. more fun like this. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

We are modifying through read-only/shared array views and this is invalid, unless we have interior mutability (i.e using Cell, Mutex or similar).

On the other hand the method is taking a mutable reference to self, hence we are guaranteed to be the only ones operating on the array - we are opting out of the compiler safety guarantees because we know that two slices are not overlapping and that we have a unique handle on the data, hence the references are valid and to the outside world the operation should be safe.
Or am I interpreting this incorrectly?

Copy link
Member

@bluss bluss Oct 9, 2019

Choose a reason for hiding this comment

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

Modifying through an ArrayView (without the Cell exception) is invalid full stop, because we treat it as we would treat a &[T] or &T. (We could go into details and exceptions at another time, maybe there's some argument for a more lenient view?)

Copy link
Member

@bluss bluss Oct 9, 2019

Choose a reason for hiding this comment

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

x is a &T but we are modifying through it. That's not permitted.

Exceptions would come in to place if you somehow got a raw pointer out of an array view, maybe you'd then be able to make an argument. But since we go through explicit shared references here, we are at invalid full stop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the S: DataMut trait bound on the method, aren't we dealing with an ArrayViewMut, which is equivalent to a &mut [T]?
Just trying to truly understand, I am not trying to argue 😅

Copy link
Member

@bluss bluss Oct 10, 2019

Choose a reason for hiding this comment

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

Sure, no problem. The types matter and there are shared refs used on the path to the swap. If we read the rust reference document this is UB, we can't just cast from &T and mutate.

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

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

Most pressing is to fix the shuffle/swap code, so that it respects borrowing/mutability rules.

Remember “unsafe code isn’t for violating Rust’s invariants, it’s for maintaining them manually”

@@ -225,17 +226,93 @@ where
R: Rng + ?Sized,
A: Copy,
D: RemoveAxis;

/// Shuffle `self`'s slices along `axis`.
Copy link
Member

Choose a reason for hiding this comment

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

Shuffle the array's xyz along axis.

We need a name for it, not sure if it should be slices. For the .lanes(axis) method we call them lanes - but those are perpendicular to axis in that case.

Copy link
Member

@bluss bluss Oct 10, 2019

Choose a reason for hiding this comment

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

I'd be tempted to call them planes. We shuffle among the items in an AxisIter, right? Each such item is n - 1 dimensional in an ndarray.

That said it's good to have 2D explanations - how to shuffle rows and columns.

Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative name could be projections, which has the plus of not carrying an intuitive dimensionality along with it (I think 2D when I read planes).

@bluss
Copy link
Member

bluss commented Oct 10, 2019

This is a good starting point (not actually -- too technical and talks about a model that is not canon)
In Rust, a shared reference is more than a pointer.

http://smallcultfollowing.com/babysteps/blog/2017/02/01/unsafe-code-and-shared-references/

@bluss bluss changed the title Add sampling_axis_inplace and sampling_axis_inplace_using Add shuffle_axis_inplace and shuffle_axis_inplace_using Oct 10, 2019
@bluss
Copy link
Member

bluss commented Oct 10, 2019

ArrayView<Cell<..>> would actually be useful here - and is not so far off - because Cell::<T>::swap is an allowed method for all T (unlike .get() which is restricted to T: Copy). But there are other tools that should make it possible to write the equivalent code with ArrayViewMut, too.

Not sure about how to solve this. Another way would be to make one permutation as a vector of len_of(axis) and then apply that permutation identically to every lane in lanes_mut(axis). But that hinges on applying permutations quickly, and I don't know about that.

@bluss
Copy link
Member

bluss commented Oct 10, 2019

How to make an ArrayView<Cell<T>

See lines 14-15

let raw_cell_view = a.raw_view_mut().cast::<Cell<f32>>();
let cell_view = unsafe { raw_cell_view.deref_into_view() };

and mind the lifetime of the conversion. Note we go through RawArrayViewMut<T, D> into ArrayView<Cell<T>, D>.

The conversion in std that looks like this is: &mut T -> &Cell<T> https://doc.rust-lang.org/nightly/std/cell/struct.Cell.html#method.from_mut

@LukeMathWalker
Copy link
Member Author

Wouldn't #717 be enough to get this method written in safe Rust?
Both slices would be mutable, hence I could get mutable reference to their elements and swap them using std::mem::swap.

@bluss
Copy link
Member

bluss commented Oct 11, 2019

Maybe, and split at or axis iter mut could also make it possible.

With slicing I think we'd struggle too much to construct the slice argument, not sure?

@LukeMathWalker
Copy link
Member Author

I tried to get it done with slicing and I am indeed struggling to construct the slice argument 😅

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

Successfully merging this pull request may close these issues.

None yet

2 participants