Skip to content

Conversation

@matsueushi
Copy link
Contributor

Ref #215. I added a new rounding option using RoundingEmulator.jl as :emulation for testing purposes. I switched the rounding mode in the test scripts from :tight to :emulation and ran all tests, then it worked fine on my Travis builds. Please let me know if I'm missing something.

@dpsanders @krish8484

@coveralls
Copy link

coveralls commented Apr 1, 2020

Coverage Status

Coverage decreased (-0.3%) to 89.488% when pulling 276ef92 on matsueushi:rounding_test into c5246d7 on JuliaIntervals:master.

@dpsanders
Copy link
Member

Great news, thanks!

@dpsanders dpsanders marked this pull request as ready for review April 7, 2020 15:20
@dpsanders
Copy link
Member

In my initial tests this is very promising indeed, thanks @matsueushi!

I think we should just make this the default option.
cc @lbenet, @Kolaru.

end



Copy link
Member

Choose a reason for hiding this comment

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

Could you add inline comment to clarify what's going on in there ? (after some research in RoundingEmulator.jl doc I think I get it, but future us will probably be thankful to have a bit of inline doc here)

@Kolaru
Copy link
Member

Kolaru commented Apr 8, 2020

If it solves a bug (even a very unlikely one), we should definitely use this as the default.

Is there a reason to keep the :tight mode if it gives the same results? (except for the one which were previously buggy)

It looks like this implementation is falling back to the :slow rounding mode for functions that are not defined by IntervalEmulator. If :emulation is to become/replace the current default, then I think the fallback should be to :tight.

Finally it would be good to document the rounding mode, at least in the header of the setrounding.jl file.

@matsueushi
Copy link
Contributor Author

matsueushi commented Apr 9, 2020

Thanks @Kolaru, I added comments to rounding.jl.

In my understanding, the arithmetic operations provided byFastRounding are +, -, *, inv, / and sqrt, and other functions are falling to :slow. All these basic arithmetic operations are also handled by RoundingEmulator, so I guess even if we change the fallback to :tight, :slow will be used eventually.

@dpsanders
Copy link
Member

The difficult-to-read, opaque, brittle, generally disgusting and undocumented code in rounding.jl is my fault... Sorry. But that's what ends up happening with this metaprogramming stuff -- it's just really hard to read.

If anybody has a suggestion for how to clean it up, that would be great.

@dpsanders
Copy link
Member

I think this is almost ready to be merged.
Does this PR make emulation the default mode?
Maybe we should just rename emulation to tight or move tight to another word.

Although tight is not correct for very small and very large values, it is up to twice as fast as emulation for some operations, e.g. multiplication, so we might want to keep it around.

I wonder if we should actually bump the minor version number since changing to emulation would technically be a breaking change. Or you could just regard it as a bug fix. I propose we just bump the patch version. @matsueushi Could you bump the patch version in Project.toml please?

@matsueushi
Copy link
Contributor Author

Then can we rename the emulation to tight and move the current tight to fast?

@dpsanders
Copy link
Member

Yes, great idea!

@matsueushi
Copy link
Contributor Author

I renamed rounding modes and bumped version.

@lbenet
Copy link
Member

lbenet commented Apr 9, 2020

I think we should bump a new minor version once this is merged.

Thanks @matsueushi , really great contribution!

@matsueushi
Copy link
Contributor Author

I found the lower bound of julia compat in Project.toml of RoundingEmulator is 1.3, which is higher than 1.1 of IntervalArithmetic. What versions of julia is supported by IntervalArithmetic? 1.1 or higher?

@dpsanders
Copy link
Member

Yes I think IntervalArithmetic.jl still in principle supports 1.1 and higher?
However, I see that we are now only testing with Julia 1.3, so maybe we should either bump that up or someone should actually test it with 1.1. If we are releasing a new minor version anyway then we should increase this to at least 1.3 I would say.

I seem to remember that there was a change which means that it does not work with Julia 1.0.5; I think it was the removal of setrounding from Base (ironically, given this PR!)

@matsueushi
Copy link
Contributor Author

Can I increase the lower bound to Julia 1.3 and bump the minor version instead of the patch version? If we support 1.2, I need to make a change on RoundingEmulator because Base.significand_bits was added in 1.3.

@dpsanders
Copy link
Member

Yes go ahead. And could you please bump Recipesbase to 1.0 as well?

@matsueushi
Copy link
Contributor Author

Thanks. I'll update Recipesbase and add some comments to NEWS.md.

@matsueushi
Copy link
Contributor Author

Done. I also bumped Polynomials to 0.7 because 0.6 is incompatible with RecipeBase 1.0.

@dpsanders
Copy link
Member

Thanks very much for this great contribution @matsueushi.

If nobody has any objections, I'll merge it soon.

@lbenet
Copy link
Member

lbenet commented Apr 10, 2020

I'm definitely in favor!

@dpsanders dpsanders merged commit 81dc482 into JuliaIntervals:master Apr 14, 2020
@dpsanders
Copy link
Member

Many thanks @matsueushi!

@matsueushi matsueushi deleted the rounding_test branch April 14, 2020 23:26
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.

5 participants