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

Normalise negative zero to zero #473

Closed
dpsanders opened this issue Jun 3, 2021 · 7 comments · Fixed by #500
Closed

Normalise negative zero to zero #473

dpsanders opened this issue Jun 3, 2021 · 7 comments · Fixed by #500

Comments

@dpsanders
Copy link
Member

julia> interval(0.0, -0.0)
[0, -0]

We should definitely normalise -0.0 to 0.0.

@lucaferranti
Copy link
Member

Section 12.12.8 Numeric functions of intervals

In formats that have a signed zero, a Level 1 value of 0 shall be returned as −0 by inf, and +0 by all other
functions in this subclause

so if I understand correctly we should map 0 to +0.0 except for inf which should return -0.0

@lucaferranti
Copy link
Member

but the only cases I can think of in practice when this would matter are

  1. printing, like in the example you posted
  2. when using ===

@lbenet
Copy link
Member

lbenet commented Jun 3, 2021

A (naive) possible implementation (for printing):

julia> myprint(x) = !iszero(x) ? x : zero(x)
myprint (generic function with 1 method)

julia> myprint(0.0)
0.0

julia> myprint(-0.0)
0.0

@lucaferranti
Copy link
Member

lucaferranti commented Jun 4, 2021

The ITL test suite has indeed

    inf [0.0,infinity] = -0.0;
    inf [-0.0,infinity] = -0.0;
    inf [-0.0,0.0] = -0.0;
    inf [0.0,-0.0] = -0.0;
    inf [0.0,0.0] = -0.0;
    inf [-0.0,-0.0] = -0.0;

so maybe we should define

    inf(x::Interval) = iszero(x.lo) ? -zero(x.lo) : x.lo
    sup(x::Interval) = iszero(x.hi) ? zero(x.hi) : x.hi

I don't recall the details of printing, but if inf and sup are used to access the boundaries of the interval, than this would fix that too.

This also strengthens my idea that we should use === in test, mainly for two reasons: 1) detect this corner case, 2) avoid checking decoration of intervals separately for decorated intervals.

at this point the fact that Interval(0.0, 0.0) === Interval(-0.0, 0.0) evaluates to false would be a feature and not a bug.

@lucaferranti
Copy link
Member

lucaferranti commented Jun 4, 2021

WoW this opened Pandora's box! I generated the tests in ITF1788.jl using === and now there are many more tests failing because of that, e.g.

julia> /(interval(-Inf,-15.0), interval(3.0,Inf))
[-∞, -0]

expected value Interval(-Inf, 0.0)

and

julia> cancelplus(interval(1.0, 5.0), interval(-5.0,-1.0))
[-0, 0]

expected value Interval(0.0, 0.0)

one alternative could be to use inf and sup (defined as above) in the tests instead of directly comparing LHS and RHS with == or ===

@Kolaru
Copy link
Collaborator

Kolaru commented Jun 4, 2021

I think we should rather check for 0 during interval construction, I feel like inf end sup should really just be equivalent to x.lo and x.hi for standard intervals.

@lucaferranti
Copy link
Member

Adding this check to interval would fix the tests with inf and sup, but I think it wouldn't solve the problem with other functions, which use Interval internally, which does not make any check.

I think fixing that would require that the check for zero is performed also by internal constructors (which maybe is what you meant from the beginning, apologies if I missed it)

On the other side, I think we don't want internal constructors to perform validity checks, so maybe we should have Interval perform the check on zero and interval the validity check? (if nothing else, this highlights the importance of thinking carefully #468 )

Ideally, I think rounding functionalities should take care of mapping 0 to -0.0 with RoundDown and to 0.0 with RoundUp, but I'm not sure if it's possible though 🤔

@lucaferranti lucaferranti linked a pull request Dec 14, 2021 that will close this issue
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.

4 participants