FIX #410 by changing type arguments passed to rationalize() on atomic()

[cnaak]

The one-line change is believed to solve Issue #410. If preferred, the one-liner can be made into a full if ... end block. Let me know!

[coveralls]

Coverage remained the same at 91.123% when pulling 2740606 on cnaak:fix_#410 into f80b683 on JuliaIntervals:master.

[Kolaru]

Thanks a lot for the contribution! Here are some feedback.

First, we usually avoid conditional on type if not necessary (I do not think such conditional are bad, but that is how the convention goes) . Here this means adding a method to atomic for interval with BigFloat parameter.

In the present case however, I wonder if using rationalize(BigInt, x) in all cases would be acceptable (@dpsanders). On my machine this incurs a 10x speed penalty (6.7 us instead of 500 ns), but atomic should never be a bottleneck anyway. Also this would remove the need for the corner-case check that follow.

Finally, could you add some tests that would fail without your fix? That makes sure we will never reintroduce the bug in the future.

[cnaak]

Dear Benoît, You have valid points. Indeed the 'julian' way of implementing cases like this is through the type system rather than the embedded `if`. I'll change the PR (Pull Request) somtime along this (busy) week, (i) doing it in the 'julian' way, which will keep the speed unchanged for the other DataTypes, and also (ii) adding pertinent tests. Best regards,

[dpsanders]

A good way to do this would be to make an internal _rationalize function that chooses the correct integer type to call rationalize with. [Note that you can pass an integer type as the first argument to rationalize (as I only just discovered!).]

[dpsanders]

We do not use capitalized variable names in the codebase.

[cnaak]

OK. Will change.

[dpsanders]

Please change this to use dispatch with appropriate methods.

[cnaak]

Done! PR updated with (i) more specialized

atomic(::Type{Interval{BigFloat}}, x::S) where {S<:AbstractFloat}

method definition and (ii) tests for the ± and .. interval construction operators with BigFloat precision arguments (using lowercase variables only).

[dpsanders]

There's no need to copy and paste this code (that is almost always a bad idea).
The only thing that needs doing is to make a function _rationalize that knows what type of Float is passed in and calls rationalize with the corresponding integer type.

[cnaak]

Agreed with being almost always a bad idea copying/pasting code like that.

I understood your last reply (of using dispatch) as applying to atomic() rather than to rationalize(). The originally intended one is now abundantly clear.

The _rationalize() method requires that some calling methods to be changed from rationalize(...) to _rationalize(...).

Here's the list (on master's) of the sources that currently call rationalize():

IntervalArithmetic.jl$ grep rationalize $(find . -name '*jl')
./test/interval_tests/construction.jl:    #@test Interval{Rational{Int}}(pi) == Interval(rationalize(1.0*pi))
./test/interval_tests/construction.jl:        @test Interval(1//10).lo == rationalize(0.1)
./src/decorations/intervals.jl:#     convert(DecoratedInterval{T}, rationalize(x))
./src/intervals/conversion.jl:    xrat = rationalize(x)
./src/intervals/conversion.jl:    Interval(rationalize(T, x))
./src/intervals/conversion.jl:    Interval(rationalize(T, x))

I imagine that the tests should be left alone, as in them, rationalize() is not being used in Interval construction and calls are part of RHS's — hence, expected values.

The cases in conversion.jl are pretty clear.

What about the one in ./src/decorations/intervals.jl? Should be left untouched or changed to _rationalize()? (I'm unfamiliar with that part of the code and it's functionality).

Best regards!

[cnaak]

Oh, in the case that rationalize() calls on multiple sources are to be changed, in which source file would be best to define _rationalize() in your estimate?