-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
Test failure on 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. |
Flaky test: #56748 |
Gentle bump |
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. |
@nanosoldier |
The package evaluation job you requested has completed - possible new issues were detected. Report summary❗ Packages that crashed6 packages crashed only on the current version.
34 packages crashed on the previous version too. ✖ Packages that failed31 packages failed only on the current version.
3347 packages failed on the previous version too. ✔ Packages that passed tests33 packages passed tests only on the current version.
5629 packages passed tests on the previous version too. ➖ Packages that were skipped altogether1323 packages were skipped on the previous version too. |
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.
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.
f72f0dd
to
5fd03bb
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
Currently,
Base
definessimilar
forBase.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 asStaticArrays.SOneTo
that exist in the ecosystem.Base
doesn't provide a method to handle a combination of different one-based ranges insimilar
, which leaves the packages in an awkward position: they need to define methods likewhere
HeterogeneousShapeTuple
is defined asTuple{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 handlesUnion{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 inBase
. This PR is a first step in this direction. We add the abstract typeAbstractOneTo
, and haveOneTo
be its subtype. We also add methods tosimilar
andreshape
that acceptAbstractOneTo
arguments. This makes it unnecessary for packages to dispatch on awkward combinations ofUnion{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 aOneTo
, but these may be addressed in separate PRs. Also, there may be one-based ranges that can't subtypeAbstractOneTo
, 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.