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

Release 0.10.1 breaking for allowmissing arrays #209

Closed
rofinn opened this issue Jan 28, 2021 · 3 comments · Fixed by #211
Closed

Release 0.10.1 breaking for allowmissing arrays #209

rofinn opened this issue Jan 28, 2021 · 3 comments · Fixed by #211

Comments

@rofinn
Copy link
Member

rofinn commented Jan 28, 2021

Looks like the latest patch release broke the ability to pass arrays with eltype Union{<:Real, Missing}

On 0.10.0:

julia> using Distances, Missings

julia> nrmsd(allowmissing(rand(10, 4)), rand(10, 4))
0.34261077849743554

But on 0.10.1:

julia> using Distances, Missings

julia> nrmsd(allowmissing(rand(10, 4)), rand(10, 4))
ERROR: MethodError: no method matching oneunit(::Type{Any})
Closest candidates are:
  oneunit(::Type{Union{Missing, T}}) where T at missing.jl:104
  oneunit(::Type{T}) where T at number.jl:300
  oneunit(::T) where T at number.jl:299
  ...
Stacktrace:
 [1] oneunit(::Type{Any}) at ./missing.jl:105
 [2] _eval_start(::SqEuclidean, ::Type{Any}, ::Type{Float64}, ::Nothing) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:320
 [3] _eval_start(::SqEuclidean, ::Type{Any}, ::Type{Float64}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:318
 [4] eval_start(::SqEuclidean, ::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:317
 [5] _evaluate at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:250 [inlined]
 [6] SqEuclidean at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:328 [inlined]
 [7] sqeuclidean(::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:353
 [8] (::MeanSqDeviation)(::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:593
 [9] (::RMSDeviation)(::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:596
 [10] (::NormRMSDeviation)(::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:601
 [11] nrmsd(::Array{Union{Missing, Float64},2}, ::Array{Float64,2}) at /Users/rory/.julia/packages/Distances/glUtY/src/metrics.jl:603
 [12] top-level scope at REPL[4]:1

I'm not sure if this was intended, but it might be good to push those kinds of changes to a breaking release?

@dkarrasch
Copy link
Member

This was, of course, not intentional, but invisible because we don't have any tests that cover this behavior. I'll look into it.

@dkarrasch
Copy link
Member

I guess we found a use case for the fallback that we decided to remove because nobody knew what it was good for. This can be fixed, in principle, by implementing the Float64 fallback again. The question is, how to do that without getting into ambiguity hell... 🤣

@rofinn
Copy link
Member Author

rofinn commented Jan 28, 2021

This was, of course, not intentional, but invisible because we don't have any tests that cover this behavior.

Sorry, wasn't trying to imply anything bad, I just wasn't sure if this was one of the edge cases where it isn't worth making folks update the deps since it's pre-1.0. Yeah, the missings use-case is pretty easy to miss 👍🏻

I'm having difficulty following all the changes/discussions between 0.10.0 to 0.10.1 would you mind posting a link to the commit/PR/discussion where the fallback was dropped? I doubt I'll have much to contribute, but it'd nice to following along. FWIW, I'm fine with calling disallowmissing moving forward since my code would error if a missing was returned anyways.

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 a pull request may close this issue.

2 participants