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

Handle zero on arrays of unions of number types and missings #53602

Merged
merged 3 commits into from
Mar 9, 2024

Conversation

oxinabox
Copy link
Contributor

@oxinabox oxinabox commented Mar 5, 2024

I believe this resolves #53582

The intent of #51458
was to only change cases that were errors into successes.
(or in case of undef elements into different errors)
Not to change behavour of any existing non-erroring code.

This particular interaction was the result of the existance of

 [19] zero(::Type{Union{Missing, T}}) where T
     @ missing.jl:105

Which only uses the T to determine the zero, not the whole type.
Which could not be replicated by iterting over the elements, as those all have concrete types.
This just pushed that check up to also happen at the AbstractArray level.
Which isn't particularly satifying, but does fix the issue, and I suspect all similar cases that actually exist in the wild.
(I will note that we do not require the same for nothing as that does not have a similar zero(Union{Nothing, T}) overload.)

Possibly there are other cases involving Vector{T}(undef,...) that also now error, which this PR does not fix.
These would exist if there is a non-isbits type that implements zero{T}, but which does not implement zero(::AbstractArray{T}), of which AFAICT there are none in Base from looking at methods zero.
In general arrays containing undef break for a ton of operations, so I didn't worry about them to much in the original PR.

@@ -1218,6 +1218,7 @@ end
copymutable(itr) = collect(itr)

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

Choose a reason for hiding this comment

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

Arguably this should be:

Suggested change
zero(x::AbstractArray{S}) where {T<:Number, S<:Union{Missing, T}} = fill!(similar(x, typeof(zero(S))), zero(S))
zero(x::AbstractArray{<:Union{Missing, T}}) where {T<:Number} = fill!(similar(x), zero(T))

which would mean that you continue to get back a type which can be mutated to hold missings.
However that is not the behavour of 1.10.
So I didn't do that.

@oxinabox
Copy link
Contributor Author

oxinabox commented Mar 5, 2024

Weirdly this doesn't seem to solve the issue for the actual OP of

Even though it does for the seemingly identical case in the tests and for the second comment on that issue

julia> @which zero(Vector{Union{Quantity{Float64, 𝐋}, Missing}}(undef, 1))
zero(x::AbstractArray)

julia> @which zero(Vector{Union{CustomNumber, Missing}}(undef, 1))
zero(x::AbstractArray{S}) where {T<:Number, S<:Union{Missing, T}}

And I am blanking on why.
Since

julia> Quantity{Float64, 𝐋} |> supertype |> supertype
Number

@oxinabox oxinabox changed the title Handle zero on arrays of unions of custom number types and missings Handle zero on arrays of unions of number types and missings Mar 5, 2024
Comment on lines +1978 to +1983
struct CustomNumber <: Number
val::Float64
end
Base.zero(::Type{CustomNumber}) = CustomNumber(0.0)
@test zero([CustomNumber(5.0)]) == [CustomNumber(0.0)]
@test zero(Union{CustomNumber, Missing}[missing]) == [CustomNumber(0.0)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

arguably we do not need this, since the error is replicated with just the Union{Float64, Missing} case above

@vtjnash
Copy link
Member

vtjnash commented Mar 5, 2024

[19] zero(::Type{Union{Missing, T}}) where T @ missing.jl:105

This is not the correct way to write this (it fails the need_to_handle_undef_sparam test, though we mark it as test_broken there). The correct way (with corrected sparam handling and usually better morespecific precedence) seems to be:

zero(::Type{T}) where {T>:Missing} = zero(nonmissingtype_checked(T))

This likely also needs to still specifically handle zero(Any) calls, since "could not compute non-missing type" is not the most useful error message:

zero(::Type{Any}) = throw(MethodError(zero, (Any,)))

The method here is similarly then possibly:

zero(x::AbstractArray{T}) where {T<:Union{Missing,Number}} = fill!(similar(x), zero(T))

@oxinabox
Copy link
Contributor Author

oxinabox commented Mar 6, 2024

Should I go and change all of these:

julia/base/missing.jl

Lines 103 to 116 in 90d84d4

for f in (:(Base.zero), :(Base.one), :(Base.oneunit))
@eval ($f)(::Type{Missing}) = missing
@eval function $(f)(::Type{Union{T, Missing}}) where T
T === Any && throw(MethodError($f, (Any,))) # To prevent StackOverflowError
$f(T)
end
end
for f in (:(Base.float), :(Base.complex))
@eval $f(::Type{Missing}) = Missing
@eval function $f(::Type{Union{T, Missing}}) where T
T === Any && throw(MethodError($f, (Any,))) # To prevent StackOverflowError
Union{$f(T), Missing}
end
end

Seems like a seperate PR?

@vtjnash
Copy link
Member

vtjnash commented Mar 6, 2024

Seems worth doing, though yes, as a separate PR probably

@KristofferC
Copy link
Member

Does Unitful pass after this PR (or at least the previous errors relating to this are no longer there)?

@oxinabox
Copy link
Contributor Author

oxinabox commented Mar 8, 2024

It does not, see:
#53602 (comment)

base/abstractarray.jl Outdated Show resolved Hide resolved
Co-authored-by: Jeff Bezanson <[email protected]>
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Mar 8, 2024
@IanButterworth IanButterworth merged commit fc6c618 into JuliaLang:master Mar 9, 2024
8 checks passed
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Mar 12, 2024
@@ -1218,6 +1218,7 @@ end
copymutable(itr) = collect(itr)

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

Choose a reason for hiding this comment

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

Could we make this a map_defined(zero, x) where mapdefined is the same as map, but adds an isdefined check in the _collect loop? Seems like we might need to define a isdefined(::Generator) check and then a iterateskip(::Generator) function for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could.
My thoughts on that when i initially did this was that it wasn't worth doing.
Because in the greater context of why you are callign zero on a AbstractArray,
you are doing linear algebra.
And other linear algebra operations like + and scalar multiplication, also error when you do them on an array containing any undefined elements.

KristofferC pushed a commit that referenced this pull request Mar 15, 2024
@KristofferC KristofferC mentioned this pull request Mar 15, 2024
60 tasks
KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 18, 2024
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 on uninitialized arrays now fail in some cases
6 participants