-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: deprecate cartesianmap
#12716
RFC: deprecate cartesianmap
#12716
Conversation
|
The commit contains a 1-line proof-by- |
👍 from me. Cartesian iteration was developed before #10380, which is why it doesn't use tuples. We can change that now, but I suspect it's better as 0.5 material. |
Agreed. Actually there are 2 interesting issues there. First, tuples don't have the correct sort order: |
Life would be so much easier if mathematics weren't column-major. |
does this "mathematics" thing have an upstream bugtracker we can report to ? |
if only |
+1 |
@@ -1132,7 +1132,8 @@ function show_nd(io::IO, a::AbstractArray, limit, print_matrix, label_slices) | |||
end | |||
tail = size(a)[3:end] | |||
nd = ndims(a)-2 | |||
function print_slice(idxs...) |
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.
You'll want to change the return
s in this function to continue
since it's now a for loop.
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.
Right you are, thanks.
👍 |
2c89af3
to
15ad7e9
Compare
RFC: deprecate `cartesianmap`
This function is redundant with
CartesianRange
, and is also a performance trap. We could eventually fix the performance, but I think there are enough other problems with this function. It's not really amap
, but more like acartesian_foreach
, so it's misnamed plus its behavior is a bit odd. I think in the future we will want to use product iterators and/or CartesianRange instead.It's a bit awkward that CartesianRange provides a CartesianIndex instead of a tuple. I'm not sure what to do about that, but it seems OK for now.