Make DomainError more informative #22751

timholy · 2017-07-11T09:59:16Z

Fixes #12152.

timholy · 2017-07-11T10:19:38Z

The travis errors look weird. A consequence of the switch to trusty?

Perhaps this will fail too, but worth a try: @nanosoldier runbenchmarks(ALL, vs = ":master")

fredrikekre · 2017-07-11T11:21:55Z

A consequence of the switch to trusty?

Perhaps, same error in #22750

tkelman · 2017-07-11T12:05:04Z

Complete coincidence, was happening before changing .travis.yml too. The unicode consortium might be rate-limiting Travis' IP's like they were doing for appveyor at one point. It looks like we maybe missed this download from our caching server whitelist, which should be added in staticfloat/cache.julialang.org#16. I'm not clear on the details of how that gets deployed though, at least since the caching server was redone and moved a couple months back.

tkelman · 2017-07-11T12:16:26Z

base/intfuncs.jl

@@ -159,6 +160,10 @@ invmod(n::Integer, m::Integer) = invmod(promote(n,m)...)
 # ^ for any x supporting *
 to_power_type(x::Number) = oftype(x*x, x)
 to_power_type(x) = x
+@noinline throw_depbs(p) = throw(DomainError(p,


this only occurs twice, may as well spell out the name a little more

tkelman · 2017-07-11T12:18:46Z

base/linalg/eigen.jl

@@ -436,6 +438,14 @@ julia> eigvecs(A, B)
 """
 eigvecs(A::AbstractMatrix, B::AbstractMatrix) = eigvecs(eigfact(A, B))

+function throw_de_complex(A)
+    if length(A) < 10


bit arbitrary, maybe the showerror should be looking at the iocontext limits?

tkelman · 2017-07-11T12:20:46Z

base/math.jl

@@ -27,6 +27,16 @@ using Core.Intrinsics: sqrt_llvm

 const IEEEFloat = Union{Float16, Float32, Float64}

+@noinline function throw_complex_domainerror(x, f)


maybe x and f should be the other way around to match nan_dom_err ?

edit: misread - maybe change to a different argument name than f for whatever it is that nan_dom_err is doing?

tkelman · 2017-07-11T12:30:53Z

base/special/trig.jl

@@ -169,7 +169,7 @@ Compute ``\\cos(\\pi x)`` more accurately than `cos(pi*x)`, especially for large
 function cospi(x::T) where T<:AbstractFloat
    if !isfinite(x)
        isnan(x) && return x
-        throw(DomainError())
+        throw(DomainError(x, "`x` must be finite."))


this and sinpi(x::T) where T<:AbstractFloat should probably be consistent

nanosoldier · 2017-07-11T13:18:28Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

timholy · 2017-07-11T15:22:01Z

We should cancel all remaining builds until #22133 (comment) is resolved, otherwise we're just clogging the queue.

tkelman · 2017-07-12T02:10:24Z

base/boot.jl

+struct DomainError <: Exception
+    val
+    msg
+    DomainError(val::ANY) = (@_noinline_meta; new(val))


keep an eye on whether @nospecialize gets merged before this

timholy · 2017-07-12T02:12:57Z

The AppVeyor failures in the old buildlogs looked fine---the failure seemed pretty clearly unrelated. OK to merge now?

tkelman · 2017-07-12T02:22:11Z

base/replutil.jl

+    if isa(ex.val, AbstractArray)
+        compact = get(io, :compact, true)
+        limit = get(io, :limit, true)
+        print(IOContext(io, compact=compact, limit=limit), "DomainError with ", ex.val)


the compact code path doesn't look like it's tested anywhere

Sacha0 · 2017-07-12T03:55:57Z

base/intfuncs.jl

@@ -147,7 +147,8 @@ julia> invmod(5,6)
 """
 function invmod(n::T, m::T) where T<:Integer
    g, x, y = gcdx(n, m)
-    (g != 1 || m == 0) && throw(DomainError())
+    g != 1 && throw(DomainError((n, m), "Greatest common divisor is 1."))


I am confused: Is g not the gcd?

Sacha0 · 2017-07-12T04:02:23Z

base/math.jl

+@noinline function throw_exp_domainerror(x)
+    throw(DomainError(x, string("Exponentiation yielding a complex result requires a ",
+                                "complex argument.\nReplace x^y with (x+0im)^y, ",
+                                "Complex(x)^y, or similar.")))


The first method recommends calling complex, whereas the second recommends calling Complex; should these methods consistently recommend one or the other?

Sacha0 · 2017-07-12T04:12:18Z

base/special/trig.jl

@@ -106,7 +106,7 @@ Compute ``\\sin(\\pi x)`` more accurately than `sin(pi*x)`, especially for large
 function sinpi(x::T) where T<:AbstractFloat
    if !isfinite(x)
        isnan(x) && return x
-        throw(DomainError())
+        throw(DomainError(x, "`x` cannot be infinite."))


Slight inconsistency between errors in this file? ("x cannot be infinite" versus "x must be finite"?)

I believe that's intentional, as some of the methods check for isnan and return the input, others don't

Sacha0

Fantastic! :)

timholy mentioned this pull request Jul 11, 2017

julep: replace InexactError with something more informative #18521

Closed

tkelman added the needs news A NEWS entry is required for this change label Jul 11, 2017

tkelman reviewed Jul 11, 2017

View reviewed changes

timholy force-pushed the teh/domain_error branch from a1405a9 to 5323835 Compare July 11, 2017 13:48

timholy closed this Jul 11, 2017

timholy reopened this Jul 11, 2017

timholy mentioned this pull request Jul 11, 2017

Make OverflowError more informative #22761

Merged

tkelman removed the needs news A NEWS entry is required for this change label Jul 12, 2017

tkelman reviewed Jul 12, 2017

View reviewed changes

Sacha0 reviewed Jul 12, 2017

View reviewed changes

Sacha0 approved these changes Jul 12, 2017

View reviewed changes

timholy added 2 commits July 12, 2017 03:28

Make DomainError more informative

d4229be

Better failure diagnostics for error paths in replutil tests

c437fc4

timholy force-pushed the teh/domain_error branch from 5323835 to c437fc4 Compare July 12, 2017 08:43

timholy merged commit 51b837b into master Jul 12, 2017

timholy deleted the teh/domain_error branch July 12, 2017 11:57

tkelman mentioned this pull request Jul 12, 2017

WIP: Better DomainError's #9693

Closed

fredrikekre mentioned this pull request Aug 5, 2017

TODO after 0.7: DomainError() should be equivalent to DomainError(nothing) #23132

Closed

Nosferican mentioned this pull request Mar 14, 2018

Native implementations for distrs JuliaStats/StatsFuns.jl#37

Open

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DomainError more informative #22751

Make DomainError more informative #22751

timholy commented Jul 11, 2017

timholy commented Jul 11, 2017

fredrikekre commented Jul 11, 2017

tkelman commented Jul 11, 2017

tkelman Jul 11, 2017

tkelman Jul 11, 2017

tkelman Jul 11, 2017 •

edited

Loading

tkelman Jul 11, 2017

nanosoldier commented Jul 11, 2017

timholy commented Jul 11, 2017

tkelman Jul 12, 2017

timholy commented Jul 12, 2017

tkelman Jul 12, 2017

Sacha0 Jul 12, 2017

Sacha0 Jul 12, 2017

Sacha0 Jul 12, 2017

tkelman Jul 12, 2017

Sacha0 left a comment

		@@ -27,6 +27,16 @@ using Core.Intrinsics: sqrt_llvm

		const IEEEFloat = Union{Float16, Float32, Float64}

		@noinline function throw_complex_domainerror(x, f)

Make DomainError more informative #22751

Make DomainError more informative #22751

Conversation

timholy commented Jul 11, 2017

timholy commented Jul 11, 2017

fredrikekre commented Jul 11, 2017

tkelman commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanosoldier commented Jul 11, 2017

timholy commented Jul 11, 2017

Choose a reason for hiding this comment

timholy commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

tkelman Jul 11, 2017 •

edited

Loading