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

Functions not compatible with all Base-compatible types #17

Open
mkborregaard opened this issue May 22, 2017 · 11 comments
Open

Functions not compatible with all Base-compatible types #17

mkborregaard opened this issue May 22, 2017 · 11 comments

Comments

@mkborregaard
Copy link
Contributor

mkborregaard commented May 22, 2017

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?

@mlubin
Copy link
Collaborator

mlubin commented May 22, 2017

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.

@mkborregaard
Copy link
Contributor Author

mkborregaard commented May 23, 2017

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.

@mlubin
Copy link
Collaborator

mlubin commented May 23, 2017

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.

@mkborregaard
Copy link
Contributor Author

Right :-/

@mkborregaard
Copy link
Contributor Author

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

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

@BobPortmann
Copy link

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.

@mlubin
Copy link
Collaborator

mlubin commented Jul 21, 2017

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.

@BobPortmann
Copy link

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.

@mlubin
Copy link
Collaborator

mlubin commented Jul 21, 2017

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.

@BobPortmann
Copy link

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

@mkborregaard
Copy link
Contributor Author

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 (#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.

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

No branches or pull requests

3 participants