- Notifications
You must be signed in to change notification settings - Fork338
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
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…_using` to`RandomExt`.
let slice1 = self.index_axis(axis, i); | ||
let slice2 = self.index_axis(axis, j); | ||
for (x, y) in slice1.iter().zip(slice2.iter()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
LukeMathWalkerOct 9, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
There was a problem hiding this 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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 commentedOct 10, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This is a good starting point (not actually -- too technical and talks about a model that is not canon) http://smallcultfollowing.com/babysteps/blog/2017/02/01/unsafe-code-and-shared-references/ |
sampling_axis_inplace
andsampling_axis_inplace_using
shuffle_axis_inplace
andshuffle_axis_inplace_using
Not sure about how to solve this. Another way would be to make one permutation as a vector of |
bluss commentedOct 10, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
How to make an See lines 14-15 Lines 14 to 15 in6b38810
and mind the lifetime of the conversion. Note we go through The conversion in std that looks like this is: |
Wouldn't#717 be enough to get this method written in safe Rust? |
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? |
I tried to get it done with slicing and I am indeed struggling to construct the slice argument 😅 |
Other two simple functions that I think might useful when working with arrays.
Should we add also
sampling_axis
andsampling_axis_using
as convenient shims for?