Skip to content

stricter shapes for setindex#59025

Open
adienes wants to merge 31 commits intoJuliaLang:masterfrom
adienes:undotted_setindex
Open

stricter shapes for setindex#59025
adienes wants to merge 31 commits intoJuliaLang:masterfrom
adienes:undotted_setindex

Conversation

@adienes
Copy link
Copy Markdown
Member

@adienes adienes commented Jul 17, 2025

not now ambitious enough to claim this fixes #40018, but let's just say I wrote it while reading the issue

fixes JuliaSparse/SparseArrays.jl#569
updates #45374 (closes?)

@giordano
Copy link
Copy Markdown
Member

not ambitious enough to claim this fixes #40018

By writing that you automatically marked this PR to close that issue anyway. If you don't want to do that, put something between "fixes" and "#40018", like "issue".

@adienes adienes added the needs tests Unit tests are required for this change label Jul 18, 2025
@adienes adienes added needs docs Documentation for this change is required needs pkgeval Tests for all registered packages should be run with this change labels Jul 18, 2025
@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 18, 2025

I (believe) this implements the idea in #40018 (comment)

I really hope it isn't a performance trainwreck

edit: BitArray needs fixing

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 19, 2025

JuliaSparse/SparseArrays.jl#637

would fix the sparsearrays test case

looks like there was some previous discussion in #43644

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 20, 2025

@nanosoldier runtests()

@adienes adienes added the arrays [a, r, r, a, y, s] label Jul 20, 2025
@nanosoldier

This comment was marked as outdated.

@adienes adienes removed the needs tests Unit tests are required for this change label Jul 22, 2025
@adienes

This comment was marked as duplicate.

@nanosoldier
Copy link
Copy Markdown
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Report summary

✖ Packages that failed

9 packages failed only on the current version.

  • Package has test failures: 1 packages
  • Package tests unexpectedly errored: 5 packages
  • Test duration exceeded the time limit: 3 packages

16 packages failed on the previous version too.

✔ Packages that passed tests

3 packages passed tests only on the current version.

  • Other: 3 packages

58 packages passed tests on the previous version too.

~ Packages that at least loaded

3 packages successfully loaded only on the current version.

  • Other: 3 packages

24 packages successfully loaded on the previous version too.

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 23, 2025

that's better. so there are ~5 real failures, all of which are trying to drop non-trailing singleton dimensions besides Vlasiator.jl, which gets
DimensionMismatch: tried to assign 3×8192 array to 3×32×16×16 destination

FYI I tried to outline the error path, but it looks like there are bootstrap issues with @noinline to annotate functions, so I had to remove it.

@adienes adienes marked this pull request as ready for review July 23, 2025 11:50
@adienes adienes removed the needs pkgeval Tests for all registered packages should be run with this change label Jul 23, 2025
@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 31, 2025

triage also considered the fact that this implementation would allow I or X in an A[I] = X expression to have differently-offset axes so long as the size matches the rule here, and was cautiously ok with that

@jishnub
Copy link
Copy Markdown
Member

jishnub commented Jul 31, 2025

Just to clarify: ignoring the axes offset in setindex! seems to be what we already do (#45374), and this PR doesn't change anything here?

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Jul 31, 2025

correct; this PR should not change the behavior of the MWE there

julia> a = [1:7;];

julia> a[Base.IdentityUnitRange(3:4)] = 2:3
2:3

although it may add documentation clarifying that this is allowed

Just something to get the docs discussion started
@mbauman
Copy link
Copy Markdown
Member

mbauman commented Jul 31, 2025

I threw a commit on here that tries to document this; it's just meant to be a starting point.

@adienes adienes removed the needs docs Documentation for this change is required label Aug 3, 2025
@adienes
Copy link
Copy Markdown
Member Author

adienes commented Aug 3, 2025

not to add another wrinkle but should this PR also remove the require_one_based_indexing here ?

require_one_based_indexing(X)

if we are going to really commit to "sizes must match, axes need not" decision

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Aug 8, 2025

I will need help coordinating with JuliaSparse/SparseArrays.jl#637

@adienes
Copy link
Copy Markdown
Member Author

adienes commented Mar 27, 2026

I'm going to run a bunch of benchmarks for setindex! across various sizes / dimensionalities / array types / eltypes etc. and if I don't see any regressions or hear any new feedback about the added docs / news, I'll follow the plan in JuliaSparse/SparseArrays.jl#637 (comment) to get it merged

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

Labels

arrays [a, r, r, a, y, s] minor change Marginal behavior change acceptable for a minor release status: waiting for PR reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unsuccessful assignment without any error Possibly undocumented behavior of indexing assignment to arrays

7 participants