Skip to content

Add AbstractOneTo and have OneTo be its subtype #56902

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

Merged
merged 4 commits into from
Apr 1, 2025
Merged

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Dec 24, 2024

Currently, Base defines similar for Base.OneTo, with the understanding that offset axes will be handled elsewhere. However, Base.OneTo is just one possible one-based range, and there are others such as StaticArrays.SOneTo that exist in the ecosystem. Base doesn't provide a method to handle a combination of different one-based ranges in similar, which leaves the packages in an awkward position: they need to define methods like

similar(A::AbstractArray, ::Type{T}, shape::HeterogeneousShapeTuple) where {T} = similar(A, T, homogenize_shape(shape))

where HeterogeneousShapeTuple is defined as

Tuple{Vararg{Union{Integer, Base.OneTo, SOneTo}}}

https://github.com/JuliaArrays/StaticArrays.jl/blob/07c12450d1b3481dda4b503564ae4a5cb4e27ce4/src/abstractarray.jl#L141-L146
Unfortunately, such methods are borderline type-piracy, as noted in JuliaArrays/StaticArrays.jl#1248. In particular, if the narrower Base method that handles Union{Integer, OneTo} is removed, then this method explicitly becomes pirating.

A solution to this situation is to have Base handle all one-based ranges, such that arbitrary combinations of one-based ranges hit fallback methods in Base. This PR is a first step in this direction. We add the abstract type AbstractOneTo, and have OneTo be its subtype. We also add methods to similar and reshape that accept AbstractOneTo arguments. This makes it unnecessary for packages to dispatch on awkward combinations of Union{Integer, OneTo} and custom one-based axes, as the base implementation would handle such cases already.

There may be other methods that accept an AbstractOneTo instead of a OneTo, but these may be addressed in separate PRs. Also, there may be one-based ranges that can't subtype AbstractOneTo, and a full solution that accepts such ranges as well needs to be implemented through a trait. This may also be handled in a separate PR.

@jishnub jishnub added arrays [a, r, r, a, y, s] ranges Everything AbstractRange labels Dec 24, 2024
@jishnub
Copy link
Member Author

jishnub commented Dec 26, 2024

Test failure onx86-64 apple darwin is

Error in testset Compiler/codegen:
Test Failed at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZM05NJYVY.0/build/default-grannysmith-C07ZM05NJYVY-0/julialang/julia-master/julia-3742f4f789/share/julia/Compiler/test/codegen.jl:412
  Expression: abs(#= /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZM05NJYVY.0/build/default-grannysmith-C07ZM05NJYVY-0/julialang/julia-master/julia-3742f4f789/share/julia/Compiler/test/codegen.jl:412 =# @allocated(f_dict_hash_alloc()) / #= /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-grannysmith-C07ZM05NJYVY.0/build/default-grannysmith-C07ZM05NJYVY-0/julialang/julia-master/julia-3742f4f789/share/julia/Compiler/test/codegen.jl:412 =# @allocated(g_dict_hash_alloc()) - 1) < 0.1
   Evaluated: 0.2642807983482449 < 0.1

I wonder if someone might be able to comment on why this fails?

Edit: oddly, this test passed on the second invocation, so it's unlikely to be related to this PR.

@nsajko
Copy link
Contributor

nsajko commented Jan 1, 2025

I wonder if someone might be able to comment on why this fails?

Flaky test: #56748

@jishnub jishnub requested a review from timholy January 4, 2025 05:26
@jishnub
Copy link
Member Author

jishnub commented Feb 14, 2025

Gentle bump

@nsajko nsajko added the design Design of APIs or of the language itself label Feb 15, 2025
@nsajko
Copy link
Contributor

nsajko commented Feb 15, 2025

Fixes #41946, closes #50361, also xref #40284

@jishnub
Copy link
Member Author

jishnub commented Feb 15, 2025

I'd say that this doesn't close #41946, as the scope of that had widened from the initial proposal. This is the first step to addressing the issue, but eventually, we may want to dispatch on traits, so that wrapper types may opt in to the behavior of the parents.

@jishnub
Copy link
Member Author

jishnub commented Mar 20, 2025

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

Report summary

❗ Packages that crashed

6 packages crashed only on the current version.

  • The process was aborted: 1 packages
  • Invalid LLVM IR was generated: 1 packages
  • A segmentation fault happened: 4 packages

34 packages crashed on the previous version too.

✖ Packages that failed

31 packages failed only on the current version.

  • Package has test failures: 4 packages
  • Package tests unexpectedly errored: 5 packages
  • Networking-related issues were detected: 1 packages
  • Tests became inactive: 2 packages
  • Test duration exceeded the time limit: 19 packages

3347 packages failed on the previous version too.

✔ Packages that passed tests

33 packages passed tests only on the current version.

  • Other: 33 packages

5629 packages passed tests on the previous version too.

➖ Packages that were skipped altogether

1323 packages were skipped on the previous version too.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Seems pretty reasonable to me. As you know very well, you only really find out how these changes work out when you go to adopt them in packages.

@jishnub jishnub added the needs news A NEWS entry is required for this change label Mar 26, 2025
@jishnub jishnub force-pushed the jishnub/AbstractOneTo branch from f72f0dd to 5fd03bb Compare March 29, 2025 05:03
@jishnub jishnub removed the needs news A NEWS entry is required for this change label Mar 29, 2025
@jishnub
Copy link
Member Author

jishnub commented Mar 30, 2025

@nanosoldier runbenchmarks("array", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@jishnub jishnub merged commit 1a4e094 into master Apr 1, 2025
6 of 9 checks passed
@jishnub jishnub deleted the jishnub/AbstractOneTo branch April 1, 2025 02:02
jishnub added a commit that referenced this pull request Apr 2, 2025
Since `AbstractOneTo` is meant to be subtyped by packages, this needs to
be public. This was missed out in
#56902.
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] design Design of APIs or of the language itself ranges Everything AbstractRange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants