NaNMath.jl icon indicating copy to clipboard operation
NaNMath.jl copied to clipboard

Functions not compatible with all Base-compatible types

Open mkborregaard opened this issue 7 years ago • 11 comments

We've tried replacing Base.maximum with NaNMath.maximum in Plots, as we very often deal with Vectors with NaNs in them. NaNs are e.g. used as placeholders for breaks in line segments. However, this causes the package to fail, as sometimes maximum will be called on something that is not a Vector{AbstractFloat}:

ERROR: MethodError: no method matching maximum(::Base.Generator{Array{String,1},Base.#length})
You may have intended to import Base.maximum
Closest candidates are:
  maximum(::Array{T<:AbstractFloat,1}) where T<:AbstractFloat at /Users/michael/.julia/v0.6/NaNMath/src/NaNMath.jl:85

AFAICS we cannot guarantee in advance what type gets passed to maximum. Is there a workaround for this?

mkborregaard avatar May 22 '17 21:05 mkborregaard

I'll likely accept a PR generalizing the implementations so long as they remain reasonably simple or copies of what is in Base with slight modification.

mlubin avatar May 22 '17 22:05 mlubin

Would it be out of the question to simply import the Base functions here (silencing the warnings)? In that case I should think all non-Vector{AbstractFloat} calls (which can't have NaNs) will just fall back on the Base methods. I may be missing some complexity here, of course.

mkborregaard avatar May 23 '17 04:05 mkborregaard

Yes, because if we do that then the implementations in NaNMath would globally replace the implementations in Base whenever the NaNMath package is loaded. NaNMath is meant to be opt-in only.

mlubin avatar May 23 '17 12:05 mlubin

Right :-/

mkborregaard avatar May 23 '17 13:05 mkborregaard

We're exploring a local solution along the lines of:

import NaNMath
_extrema(x...) = Base.extrema(x...)
_extrema(x::AbstractVector{<:AbstractFloat}) = NaNMath.extrema(x)

mkborregaard avatar May 25 '17 10:05 mkborregaard

Since the functions in NaNMath are not exported, it seems to me that they could follow this pattern as well, i.e. inside NaNMath add

mean(x...) = Base.mean(x...)

Also, it would be great to define the other forms of the function inside NaNMath. e. g.

mean(x::AbstractArray{:<AbstractFloat}, region) = ...

This way if someone replaces mean by nm.mean inside their code everything works.

BobPortmann avatar Jul 21 '17 18:07 BobPortmann

Well then sometimes the method ignores NaNs and sometimes it doesn't. I'd rather have a method error than a surprising behavior like that.

mlubin avatar Jul 21 '17 18:07 mlubin

I thought that only subtypes of AbstractFloat support NaN's and so that would never happen (if all code paths that take AbstractFloat are handled in NaNMath). If only Float64 and Float32 support NaN's then the functions in NaNMath could be defined only for them, other types would fallback to the Base code.

BobPortmann avatar Jul 21 '17 19:07 BobPortmann

I thought that only subtypes of AbstractFloat support NaN's and so that would never happen (if all code paths that take AbstractFloat are handled in NaNMath).

That's a reasonable argument. I would consider a PR.

mlubin avatar Jul 21 '17 19:07 mlubin

OK. I'll try to put something together in the next few weeks.

BobPortmann avatar Jul 21 '17 19:07 BobPortmann

I thought that only subtypes of AbstractFloat support NaN's and so that would never happen

Note that I had a PR implementing this based on exactly this assumption (https://github.com/mlubin/NaNMath.jl/pull/21), but it was closed because the assumption isn't strictly true. Tuples can hold NaN s as welll (see the discussion on the PR). I'd still welcome this change, though.

mkborregaard avatar Jul 24 '17 09:07 mkborregaard