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

zeros/ones/fill may accept arbitrary axes that are supported by similar #53965

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Apr 5, 2024

The idea is that functions like zeros are essentially constructing a container and filling it with a value. similar seems perfectly placed to construct such a container, so we may accept arbitrary axes in zeros as long as there's a corresponding similar method that is defined for the axes. Packages therefore would only need to define similar, and would get zeros/ones and fill for free. For example, the following will work after this:

julia> using StaticArrays

julia> zeros(SOneTo(2), 2)
2×2 Matrix{Float64}:
 0.0  0.0
 0.0  0.0

julia> zeros(SOneTo(2), Base.OneTo(2))
2×2 Matrix{Float64}:
 0.0  0.0
 0.0  0.0

Neither of these work on the current master, as StaticArrays doesn't define zeros for these combinations, even though it does define similar. One may argue for these methods to be added to StaticArrays, but this seems to be adding redundancy.

The flip side is that OffsetArrays defines exactly these methods, so adding them to Base would break precompilation for the package. However, OffsetArrays really shouldn't be defining these methods, as this is type-piracy. The methods may be version-limited in OffsetArrays if this PR is merged.

On the face of it, trues and falses should also work similarly, but currently these seem to be bypassing similar and constructing a BitArray explicitly. I have not added the corresponding methods for these functions, but they may be added as well.

@jishnub jishnub added the arrays [a, r, r, a, y, s] label Apr 5, 2024
@dkarrasch
Copy link
Member

On the face of it, trues and falses should also work similarly, but currently these seem to be bypassing similar and constructing a BitArray explicitly.

I think this is their intended functionality. In that respect, they are deliberately different from zeros(Bool, ...) and ones(Bool, ...).

@jishnub jishnub requested a review from N5N3 April 12, 2024 07:19
@N5N3
Copy link
Member

N5N3 commented Apr 12, 2024

LGTM, I think trues and falses could adopt the similar(BitArray,...) style, as there seems to be no reason not to do so.

@jishnub jishnub force-pushed the jishnub/zerosonesfillsimilar branch from 1aac152 to 420e782 Compare April 14, 2024 11:00
@N5N3 N5N3 added the merge me PR is reviewed. Merge when all tests are passing label Apr 14, 2024
@jishnub jishnub merged commit 8d577ab into master Apr 14, 2024
8 checks passed
@jishnub jishnub deleted the jishnub/zerosonesfillsimilar branch April 14, 2024 14:51
@Keno
Copy link
Member

Keno commented Apr 15, 2024

This appears to be causing some havoc across the package ecosystem due to

┌ ModelingToolkit
│  WARNING: Method definition fill(Any, Tuple{Vararg{Union{Integer, Base.AbstractUnitRange{T} where T}, N}}) where {N} in module Base at array.jl:532 overwritten in module OffsetArrays at /cache/julia-buildkite-plugin/depots/2a2f1104-7c9d-4bf6-b32f-6252e955fe73/packages/OffsetArrays/rMTtC/src/OffsetArrays.jl:394.
│  ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.
└  

Can somebody look into adjusting OffsetArrays?

Keno added a commit that referenced this pull request Apr 15, 2024
@fatteneder fatteneder removed the merge me PR is reviewed. Merge when all tests are passing label Apr 23, 2024
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]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants