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

Make zero on array of arrays etc apply recursively #51458

Merged
merged 6 commits into from
Feb 12, 2024

Conversation

oxinabox
Copy link
Contributor

Closes #38064

I wonder if this breaks things, in practice. It shouldn't. Since old code behavior errored for the cases I am aware of.
As discussed in #38064, this definition is needed to be consistent with our other linear algebra operations,
and with us considering a vector of vectors etc as a vector space.

@oxinabox oxinabox changed the title Make zero on array of arrays apply recursively Make zero on array of arrays etc apply recursively Sep 26, 2023
@@ -1198,7 +1198,8 @@ function copymutable(a::AbstractArray)
end
copymutable(itr) = collect(itr)

zero(x::AbstractArray{T}) where {T} = fill!(similar(x, typeof(zero(T))), zero(T))
zero(x::AbstractArray{T}) where {T<:Number} = fill!(similar(x, typeof(zero(T))), zero(T))
zero(x::AbstractArray) = map(zero, x)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use broadcasting instead, as custom sparse arrays might define their own broadcast styles and don't need to iterate over the entire array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe? idk

Custom sparse arrays should probably be implementing their own zero function outright.
Since they likely have their own opinions about whether or not to store structural zeros etc (the SparseArrays stdlib does this).

Copy link
Member

Choose a reason for hiding this comment

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

It's also a whole lot easier to define a single-argument map(f, ::CustomArray) than it is to define your own broadcast style. I think the map is fine; it's straightforward and obvious and even enables the laser-focused map(::typeof(zero), ::CustomArray).

@oxinabox
Copy link
Contributor Author

I do not understand the whitespace failure
https://buildkite.com/julialang/julia-master/builds/28131#018ad586-9fea-401c-b809-8aabe49f59ea/711-714



Whitespace check found 2 issues:
base/abstractarray.jl:3544 -- trailing blank lines
test/abstractarray.jl:1873 -- no trailing newline
make: *** [Makefile:129: check-whitespace] Error 1

Does it want trailing new lines or not?

D

base/abstractarray.jl Outdated Show resolved Hide resolved
test/abstractarray.jl Outdated Show resolved Hide resolved
@oxinabox
Copy link
Contributor Author

oxinabox commented Nov 2, 2023

one thing this does not handle is if the original array has undef elements.
I am not sure if it should or not.

@adienes
Copy link
Contributor

adienes commented Feb 10, 2024

this functionality was asked for on slack

possible for 1.11?

@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Feb 10, 2024
@vtjnash
Copy link
Member

vtjnash commented Feb 10, 2024

I don't forsee an issue with this, so let's merge

test/abstractarray.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash merged commit a459926 into JuliaLang:master Feb 12, 2024
4 of 7 checks passed
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Feb 12, 2024
@Jutho
Copy link
Contributor

Jutho commented Mar 3, 2024

I think it is great that this is fixed. Nonetheless, I would like to advertise https://github.com/Jutho/VectorInterface.jl which on his README lists some other potential issues (https://github.com/Jutho/VectorInterface.jl?tab=readme-ov-file#current-situation-and-problems-in-julia). My apologies if this is out of place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zero is not recursive
8 participants