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

Function(s) to express nearishness #652

Merged
merged 3 commits into from
Apr 2, 2012
Merged

Conversation

pao
Copy link
Member

@pao pao commented Mar 31, 2012

This is a first shot at expressing nearishness in Julia, taking as inspiration NumPy's allclose() function. It doesn't handle Inf/NaN the same as numpy.allclose(), and doesn't handle all the possible types that R's all.equal() can apparently handle.

So, what is needed from those other functions here (or for specific types), noting that isequal() already exists to express actual equality? The single commit is the simplest thing that could possibly work, but I'll add optimized things (like mapping it to isequal() for integer types) before merge if I'm on the right path and this is something we want.

Whether this belongs in base/ is certainly up for discussion as well.

Input from everyone certainly helpful, but I do want to make sure @dmbates gets to see this, since he got me started thinking about it.

numpy.allclose() is defined at https://github.com/numpy/numpy/blob/master/numpy/core/numeric.py#L1963

@timholy
Copy link
Member

timholy commented Apr 1, 2012

Nice idea. Two points:

  1. Your expression is not symmetric in x and y. I'd at least change it to

isclose(x, y, rtol, atol) = abs(x-y) <= atol+rtol.*(abs(x)+abs(y))

(Otherwise, you'd get very different behavior for elements where y is zero but x is not.)

  1. In the version that supplies defaults for the two parameters, it won't work well for a Float32. How about

    isclose{T}(x::T, y::T) = isclose(x, y, eps(::Type{T})^(1/3), sqrt(eps(::Type{T})))

I'm assuming that the compiler will recognize that the powers-of-eps computations result in constants, so there shouldn't be any overhead.

@ViralBShah
Copy link
Member

This does require some thought and needs to evolve. Also, support arrays, and be extensible. @pao, can you move it to extras for now?

Preparing for a 1.0 release, I believe we will be moving more stuff out of base in any case...

@timholy
Copy link
Member

timholy commented Apr 1, 2012

To be fair, his version supported arrays. My "eps cleanup" would need additional specifications for x::Array{T} and x::DArray{T}, however.

@pao
Copy link
Member Author

pao commented Apr 1, 2012

The asymmetry is also present in the NumPy prototype, so I kept it. It's open for discussion, though. I've also considered the relative tolerance expression rtol.*max(abs(x), abs(y)) which would allow for tighter tolerances than simply adding.

As @timholy observed, this does support arrays and scalar/array mixes; I intentionally used .* to ensure that. But if we want the same NaN/Inf handling as NumPy (I'm not sure what R does yet) I can't see how to do that without specializing for arrays.

@ViralBShah extras/nearequal.jl sound good?

@ViralBShah
Copy link
Member

Location sounds fine.

-viral

On 01-Apr-2012, at 7:31 PM, paoreply@reply.github.com wrote:

The asymmetry is also present in the NumPy prototype, so I kept it. It's open for discussion, though. I've also considered the relative tolerance expression rtol.*max(abs(x), abs(y)) which would allow for tighter tolerances than simply adding.

As @timholy observed, this does support arrays and scalar/array mixes; I intentionally used .* to ensure that. But if we want the same NaN/Inf handling as NumPy (I'm not sure what R does yet) I can't see how to do that without specializing for arrays.

@ViralBShah extras/nearequal.jl sound good?


Reply to this email directly or view it on GitHub:
#652 (comment)

@pao
Copy link
Member Author

pao commented Apr 1, 2012

New version incorporating some of the above suggestions. I'm letting isclose(NaN, NaN) be true, which allows isclose() to be used to check that two similar computations which may result in NaN have the same results. (EDIT: Previous sentence had a misstatement about numpy.allclose().)

Alternatively, I could let isclose(NaN, NaN) be false, and (following MATLAB's isequalwithequalnans()) provide an isclosewithequalnans() or similar.

@ViralBShah
Copy link
Member

@pao, let me know when you think this is ready for a merge. Would you like the pull request open for some more discussion? I hate those long Matlab function names, but they are actually quite clear about what they do.

@ViralBShah
Copy link
Member

isclose(nan, nan) ought to be false, but for this purpose, I guess the behaviour you implemented is what we need.

@Keno
Copy link
Member

Keno commented Apr 2, 2012

How about isclosen following MATLAB's isequaln?

@pao
Copy link
Member Author

pao commented Apr 2, 2012

Agreed on the long function names; I didn't know about isequaln(), which looks like the new name for isequalwithequalnans(). Let's have them both and get MATLAB parity here. I can implement isequaln() while I'm at it.

@ViralBShah
Copy link
Member

I didn't know that either - must be a new name. I like it better. Perhaps we can dispense with the "withequalnans" names? I don't think that anyone except Matlab geeks like us actually know about such things, but I could be wrong. :-)

-viral

On 02-Apr-2012, at 8:36 AM, pao wrote:

Agreed on the long function names; I didn't know about isequaln(), which looks like the new name for isequalwithequalnans(). Must be newish. Let's have them both and get MATLAB parity here. I can implement isequaln() while I'm at it.


Reply to this email directly or view it on GitHub:
#652 (comment)

…lementations.

Originally, isclose() treated NaNs as equal, but this conflicts with the conventional
default. Since treating NaNs as equal is often useful, that ability is preserved by
the implementation of isclosen(). Similarly, isequaln() is included as an equal-NaNs
version of isequal().
@pao
Copy link
Member Author

pao commented Apr 2, 2012

Viral, let's bake this for a few more hours. I'll see if there are any further comments when I wake up in the morning. And if someone could check that I'm doing the @eval correctly—it does work, so I think I am—that would be great.

UPDATE: I just checked the docs; isequaln() is brand spankin' new in R2012a, so I don't feel bad for not knowing that. :D

@pao
Copy link
Member Author

pao commented Apr 2, 2012

@ViralBShah Go for merge.

@ViralBShah
Copy link
Member

@JeffBezanson can check the macrology, as he calls it..

ViralBShah added a commit that referenced this pull request Apr 2, 2012
Function(s) to express nearishness
@ViralBShah ViralBShah merged commit 913171f into JuliaLang:master Apr 2, 2012
non-Jedi added a commit to non-Jedi/julia that referenced this pull request Mar 19, 2020
This reuses the machinery that gives good stacktraces when a user
calls wait on a Task. Since bind internally uses _wait2, this
machinery is bypassed currently.

Currently, julia throws an uninformative stacktrace in this situation:

julia> c = Channel(_ -> error("foo"))
Channel{Any}(sz_max:0,sz_curr:0)

julia> for i in c end
ERROR: foo
Stacktrace:
 [1] iterate(::Channel{Any}) at ./channels.jl:459
 [2] top-level scope at ./REPL[2]:1

With this change, the stacktrace is much more informative:

julia> for i in c end
ERROR: TaskFailedException:
foo
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] (::var"JuliaLang#1#2")(::Channel{Any}) at ./REPL[4]:1
 [3] (::Base.var"JuliaLang#652#653"{var"JuliaLang#1#2",Channel{Any}})() at ./channels.jl:129
Stacktrace:
 [1] check_channel_state at ./channels.jl:167 [inlined]
 [2] take_unbuffered(::Channel{Any}) at ./channels.jl:405
 [3] take! at ./channels.jl:383 [inlined]
 [4] iterate(::Channel{Any}, ::Nothing) at ./channels.jl:449
 [5] iterate(::Channel{Any}) at ./channels.jl:448
 [6] top-level scope at ./REPL[5]:1
JeffBezanson pushed a commit that referenced this pull request Apr 30, 2020
This reuses the machinery that gives good stacktraces when a user
calls wait on a Task. Since bind internally uses _wait2, this
machinery is bypassed currently.

Currently, julia throws an uninformative stacktrace in this situation:

julia> c = Channel(_ -> error("foo"))
Channel{Any}(sz_max:0,sz_curr:0)

julia> for i in c end
ERROR: foo
Stacktrace:
 [1] iterate(::Channel{Any}) at ./channels.jl:459
 [2] top-level scope at ./REPL[2]:1

With this change, the stacktrace is much more informative:

julia> for i in c end
ERROR: TaskFailedException:
foo
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] (::var"#1#2")(::Channel{Any}) at ./REPL[4]:1
 [3] (::Base.var"#652#653"{var"#1#2",Channel{Any}})() at ./channels.jl:129
Stacktrace:
 [1] check_channel_state at ./channels.jl:167 [inlined]
 [2] take_unbuffered(::Channel{Any}) at ./channels.jl:405
 [3] take! at ./channels.jl:383 [inlined]
 [4] iterate(::Channel{Any}, ::Nothing) at ./channels.jl:449
 [5] iterate(::Channel{Any}) at ./channels.jl:448
 [6] top-level scope at ./REPL[5]:1
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.

4 participants