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

SubArray: avoid invalid elimination of singleton indices #53228

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Feb 7, 2024

close #53209

@N5N3 N5N3 added domain:arrays [a, r, r, a, y, s] backport 1.10 Change should be backported to the 1.10 release labels Feb 7, 2024
@vadim-z
Copy link

vadim-z commented Feb 7, 2024

What if the last index is not singleton but, for example, 3? I fear it would be incorrectly removed from the list J'.

@N5N3
Copy link
Member Author

N5N3 commented Feb 7, 2024

Your concern would be took care by bounds checking. See

return checkindex(Bool, get(inds, 1, OneTo(1)), I[1])::Bool &

L159 only eliminates trailing singleton indices after all parent's dimensions have been consumed.
It's safe as all of them == 1.
In fact, all of these trailing indices could be replaced with something like fill(CartesianIndex(), index_shape(trailing_indices...)) if length(index_shape(trailing_indices...)) > 0
But at present we don't have lazy fill in Base.

@vadim-z
Copy link

vadim-z commented Feb 7, 2024

Sorry, but because of CartesianIndex() bound checking will pass, and this is correct behavior.

Example:

julia> A = rand(2,3)
2×3 Matrix{Float64}:
 0.0221974  0.820445  0.861314
 0.555607   0.457571  0.875773

julia> A[[CartesianIndex()], :, 3]
1×2 Matrix{Float64}:
 0.861314  0.875773

julia> J = map(i->Base.unalias(A,i), to_indices(A, ([CartesianIndex()], :, 3)))
([CartesianIndex()], Base.Slice(Base.OneTo(2)), 3)

julia> @boundscheck checkbounds(A, J...)

julia>

If some extra axes are prepended, the trailing indices are not really "trailing", as they refer to real dimensions of a parent array.

@N5N3
Copy link
Member Author

N5N3 commented Feb 7, 2024

Yes, but CartesianIndex() also consumes no parent's dimension.
Thus rm_singleton_indices won't eliminate 3 from J = ([CartesianIndex()], :, 3))
I guess the added test

@test A[CartesianIndices(()), :, 1] == @inferred(view(A, CartesianIndices(()), :, 1))

is enough?

@vadim-z
Copy link

vadim-z commented Feb 7, 2024

Yes, you are right.

Yes, but CartesianIndex() also consumes no parent's dimension.

I've missed the point.

@N5N3 N5N3 merged commit 4d0a469 into JuliaLang:master Feb 8, 2024
5 of 7 checks passed
@N5N3 N5N3 deleted the subarray_fix branch February 8, 2024 06:22
@KristofferC KristofferC mentioned this pull request Feb 20, 2024
19 tasks
KristofferC pushed a commit that referenced this pull request Feb 26, 2024
KristofferC pushed a commit that referenced this pull request Feb 26, 2024
KristofferC added a commit that referenced this pull request Feb 27, 2024
Backported PRs:
- [x] #53205 <!-- Profile: add notes to `print()` docs -->
- [x] #53170 <!-- Remove outdated discussion about externally changing
module bindings -->
- [x] #53228 <!-- SubArray: avoid invalid elimination of singleton
indices -->
- [x] #51361 <!-- code_warntype docs: more neutral reference to
@code_warntype -->
- [x] #50480 <!-- Document --heap-size-hint in Command-line Interface
-->
- [x] #53301 <!-- Fix typo in `Sys.total_memory` docstring. -->
- [x] #53354 <!-- fix code coverage bug in tail position and `else` -->
- [x] #53388 <!-- Fix documentation: thread pool of main thread -->
- [x] #53429 <!-- Subtype: skip slow-path in `local_∀_∃_subtype` if
inputs contain no ∃ typevar. -->
- [x] #53437 <!-- Add debug variant of loader_trampolines.o -->

Need manual backport:
- [ ] #52505 <!-- fix alignment of emit_unbox_store copy -->
- [ ] #53373 <!-- fix sysimage-native-code=no option with pkgimages -->
- [ ] #53439 <!-- staticdata: fix assert from partially disabled native
code -->

Contains multiple commits, manual intervention needed:
- [ ] #52913 <!-- Added docstring for Artifacts.jl -->
- [ ] #53218 <!-- Fix interpreter_exec.jl test -->

Non-merged PRs with backport label:
- [ ] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [ ] #53125 <!-- coverage: count coverage where explicitly requested by
inference only -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Mar 12, 2024
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

view fails when new axis is prepended and last axis is selected
3 participants