Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Addshuffle_axis_inplace andshuffle_axis_inplace_using#742

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
LukeMathWalker wants to merge2 commits intorust-ndarray:master
base:master
Choose a base branch
Loading
fromLukeMathWalker:sampling

Conversation

LukeMathWalker
Copy link
Member

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

Should we add alsosampling_axis andsampling_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

@blussblussOct 9, 2019
edited
Loading

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 isinvalid, unless we have interior mutability (i.e using Cell, Mutex or similar).

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

Copy link
Member

@blussblussOct 9, 2019
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

@LukeMathWalkerLukeMathWalkerOct 9, 2019
edited
Loading

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 toself, 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

@blussblussOct 9, 2019
edited
Loading

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

@blussblussOct 9, 2019
edited
Loading

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
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

@blussblussOct 10, 2019
edited
Loading

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

@blussbluss 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 alongaxis.

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 toaxis in that case.

Copy link
Member

@blussblussOct 10, 2019
edited
Loading

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 themplanes. We shuffle among the items in an AxisIter, right? Each such item isn - 1 dimensional in an ndarray.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

bluss reacted with thumbs up emoji
@bluss
Copy link
Member

bluss commentedOct 10, 2019
edited
Loading

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/

LukeMathWalker reacted with thumbs up emoji

@blussbluss changed the titleAddsampling_axis_inplace andsampling_axis_inplace_usingAddshuffle_axis_inplace andshuffle_axis_inplace_usingOct 10, 2019
@bluss
Copy link
Member

ArrayView<Cell<..>> would actually be useful here - and is not so far off - becauseCell::<T>::swap is an allowed method for allT (unlike.get() which is restricted toT: 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 oflen_of(axis) and then apply that permutation identically to every lane inlanes_mut(axis). But that hinges on applying permutations quickly, and I don't know about that.

@bluss
Copy link
Member

bluss commentedOct 10, 2019
edited
Loading

How to make anArrayView<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 throughRawArrayViewMut<T, D> intoArrayView<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
MemberAuthor

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 usingstd::mem::swap.

@bluss
Copy link
Member

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
MemberAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@blussblussbluss requested changes

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@LukeMathWalker@bluss

[8]ページ先頭

©2009-2025 Movatter.jp