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

Use FastRounding for speed #25

Merged
merged 14 commits into from
Jun 8, 2017
Merged

Use FastRounding for speed #25

merged 14 commits into from
Jun 8, 2017

Conversation

dpsanders
Copy link
Member

@dpsanders dpsanders commented May 1, 2017

Use FastRounding.jl as a drop-in replacement for changing the rounding mode. This gives a 3-4x speed-up for basic arithmetic operations.

@codecov-io
Copy link

codecov-io commented May 2, 2017

Codecov Report

Merging #25 into master will decrease coverage by 0.65%.
The diff coverage is 68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   92.29%   91.64%   -0.66%     
==========================================
  Files          21       21              
  Lines         935      945      +10     
==========================================
+ Hits          863      866       +3     
- Misses         72       79       +7
Impacted Files Coverage Δ
src/IntervalArithmetic.jl 100% <ø> (ø) ⬆️
src/multidim/intervalbox.jl 81.25% <ø> (ø) ⬆️
src/intervals/arithmetic.jl 98.76% <100%> (ø) ⬆️
src/intervals/rounding.jl 75% <65.21%> (-1%) ⬇️
src/intervals/conversion.jl 56.52% <0%> (-13.05%) ⬇️
src/decorations/intervals.jl 86.84% <0%> (-2.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b54e103...9ab304b. Read the comment docs.

@coveralls
Copy link

coveralls commented May 20, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.159% when pulling 6e7963b on fast_rounding into 64f72c7 on master.

@coveralls
Copy link

coveralls commented May 28, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.159% when pulling c5a5e4f on fast_rounding into 0d7f112 on master.

@lbenet lbenet mentioned this pull request May 30, 2017
Copy link
Member

@lbenet lbenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is really nice. I suggest only to include docstrings, explain the restrictions of :errorfree and rename the rounding modes.


mid{T}(a::Interval{Rational{T}}) = (1//2) * (a.lo + a.hi)

function simple_mid(a::Interval)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need simple_mid as a new function? I think its sole use is inside that method for mid.

In any case, it is now good timing to add docstrings and tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that we no longer need simple_mid.

@@ -9,6 +9,7 @@ interval rounding types, e.g.
+(IntervalRounding{:none}, a, b, RoundDown)

The current allowed rounding types are
- :errorfree # use errorfree arithmetic via FastRounding.jl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest the following changes for naming the rounding modes:

  • :errorfree should be named :fast
    -:fast should be named :wider (or :wide?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also be mentioned somewhere (the docstrings and perhaps a warning too) that :errorfree works only with Float64 and Float32.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shall still refer to :errorfree instead of (the new) :fast to avoid confusions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with :fast and :wide, thanks for the suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the standard, how about :errorfree -> :fast; :fast -> :accurate; :correct-> :tight?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that this implies that :fast is not :tight, which is incorrect. (Errorfree arithmetic does give tight bounds.)

@@ -157,23 +199,31 @@ doc"""
setrounding(Interval, rounding_type::Symbol)

Set the rounding type used for all interval calculations on Julia v0.6 and above.
Valid `rounding_type`s are `:correct`, `:fast` and `:none`.
Valid `rounding_type`s are `:correct`, `:fast` and `:none`, `:errorfree`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should read as "Valid rounding_types are :correct, :fast, :none, and :errorfree".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

@lbenet
Copy link
Member

lbenet commented Jun 1, 2017

I forgot to say that this should probably be rebased against current master; the tests related to cancelminus and cancelplus should all pass.

@lbenet
Copy link
Member

lbenet commented Jun 4, 2017

I just read in the standard (cf page 9, definition of tightness) about what we are discussing as rounding modes. They use the nomenclature "tightest", "accurate" and "valid", which correspond respectively to :correct, :fast and :wide. I mention this in case we want to change the nomenclature to what appears in the standard.

@dpsanders dpsanders force-pushed the fast_rounding branch 2 times, most recently from 05b5e71 to 0228710 Compare June 5, 2017 09:12
@lbenet
Copy link
Member

lbenet commented Jun 7, 2017

What is faster, :errorfree (now :fast) or :fast (now :accurate)?

I actually thought that the bounds provided by :errorfree were tighter than using prevfloat and nextfloat, and almost as good as :correct (now :tight).

I suggest to have the docstring in decreasing order of tightness.

@dpsanders
Copy link
Member Author

What was :errorfree (now :fast) is exactly as accurate as :tight (what was :correct).
In fact, :tight (using changes of rounding mode) is now obsolete -- except to check that :fast is correct!

@dpsanders
Copy link
Member Author

So maybe we can replace :errorfree / :fast with :tight, and :correct / :tight with :slow ;)

@lbenet
Copy link
Member

lbenet commented Jun 7, 2017

Maybe you are right; give me a bit of time, since I thought i had some counterexample.

@lbenet
Copy link
Member

lbenet commented Jun 7, 2017

So maybe we can replace :errorfree / :fast with :tight, and :correct / :tight with :slow ;)

I just run all ITF1788 suite tests and they pass with :errorfree / :fast. So, I agree: it should be renamed as :tight, and :correct / :tight with :slow, and the other that involves prevfloat/nextfloat should be :fast.

Sorry for being so skeptic...

@coveralls
Copy link

coveralls commented Jun 7, 2017

Coverage Status

Coverage decreased (-0.2%) to 92.063% when pulling 28fa625 on fast_rounding into b54e103 on master.

@dpsanders
Copy link
Member Author

I was quite sceptical actually that this would ever be possible. But thanks to @JeffreySarnoff and some hard work fixing difficult corner cases, it seems that it is finally finished!

@dpsanders
Copy link
Member Author

So far, I have :tight, :accurate (the one just using prevfloat and nextfloat, for which you suggested :fast), :slow and :none. Is that OK with you?

@JeffreySarnoff
Copy link
Contributor

!

@lbenet
Copy link
Member

lbenet commented Jun 7, 2017

That's ok with me!

Once travis shows the green lights, I'll merge this.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.64% when pulling 9ab304b on fast_rounding into b54e103 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.64% when pulling 9ab304b on fast_rounding into b54e103 on master.

@coveralls
Copy link

coveralls commented Jun 8, 2017

Coverage Status

Coverage decreased (-0.7%) to 91.64% when pulling 9ab304b on fast_rounding into b54e103 on master.

@dpsanders
Copy link
Member Author

Everything is passing!

@lbenet lbenet merged commit 2ecefe5 into master Jun 8, 2017
@lbenet
Copy link
Member

lbenet commented Jun 8, 2017

This is really nice! Merging...

@dpsanders
Copy link
Member Author

Thanks!

@dpsanders dpsanders deleted the fast_rounding branch August 2, 2017 19:40
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.

None yet

5 participants