Make Interval constructor fast by not including checks #26

dpsanders · 2017-05-01T18:57:29Z

The IA_VALID environment variable now determines if validity checks are used. E.g. run Julia with

IA_VALID=true julia --compilecache=no

to turn on validity checks; by default, they are off.

codecov-io · 2017-05-02T00:53:18Z

Codecov Report

Merging #26 into master will decrease coverage by 0.41%.
The diff coverage is 81.48%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   91.64%   91.22%   -0.42%     
==========================================
  Files          21       21              
  Lines         969      980      +11     
==========================================
+ Hits          888      894       +6     
- Misses         81       86       +5

Impacted Files	Coverage Δ
src/IntervalArithmetic.jl	`100% <ø> (ø)`	⬆️
src/multidim/intervalbox.jl	`87.5% <ø> (ø)`	⬆️
src/intervals/macros.jl	`100% <100%> (ø)`	⬆️
src/intervals/arithmetic.jl	`97.51% <100%> (-0.02%)`	⬇️
src/intervals/intervals.jl	`86.48% <79.16%> (-13.52%)`	⬇️

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 e17aeef...42124cc. Read the comment docs.

coveralls · 2017-05-28T13:40:30Z

Coverage decreased (-1.08%) to 91.323% when pulling 2dd3a2e on simplify_constructor into 0d7f112 on master.

coveralls · 2017-05-28T14:17:04Z

Coverage decreased (-1.08%) to 91.323% when pulling 2dd3a2e on simplify_constructor into 0d7f112 on master.

coveralls · 2017-06-08T12:57:12Z

Coverage decreased (-3.7%) to 87.891% when pulling 8952216 on simplify_constructor into 2ecefe5 on master.

coveralls · 2017-06-08T12:57:12Z

Coverage decreased (-3.7%) to 87.891% when pulling 8952216 on simplify_constructor into 2ecefe5 on master.

lbenet · 2017-06-11T03:35:59Z

What should we do about this? This is already included through the :accurate rounding mode.

lbenet

This is very nice, thank you.

However, I dislike when the default behavior goes away from compliance with the standard. My main suggestion is to have the defaults to be closest to the standard, and only by explicit choice by the user, get away from that default behavior.

lbenet · 2017-06-22T18:39:40Z

src/intervals/intervals.jl

@@ -5,37 +5,34 @@

 ## Interval type

+const validity_check = false


I think the default value should be true, since that is the case that (eventually) will ensure compliance with the standard. This could be changed by the IA_VALID environment variable, which I guess can be set also in the .juliarc file.

Another reason to change this default is the fact that many users may construct directly the intervals using the constructor instead of interval...

lbenet · 2017-06-22T18:46:03Z

src/intervals/intervals.jl

+end
+
+interval(a::Real) = interval(a, a)
+


If I understand interval always checks the validity of the construction, while
Interval does not (currently as defualt) unless IA_VALID=true. Is this correct?

Yes, exactly: the user is supposed to use the interval function to build an Interval, rather than the Interval constructor itself. Since the interval function does perform checks, I don't think there is any problem with standards compliance.

In fact, my idea would be that the Interval constructor should not even be exported, so that it cannot be used "accidentally".

Can you also add some docstrings here?

lbenet · 2017-06-22T18:48:24Z

test/interval_tests/construction.jl

+    @test_throws ArgumentError interval(big(2), big(1))
+    @test_throws ArgumentError interval(BigInt(1), 1//10)
+    @test_throws ArgumentError interval(1, 0.1)
+    @test_throws ArgumentError interval(big(1), big(0.1))



If my comment about interval is correct (interval checks the validity and Interval may not), I think it is a good idea to test the differences among Interval and interval when their behavior differs.

EDIT: Fix the 's

lbenet · 2017-06-22T18:50:07Z

test/interval_tests/construction.jl

+#     a = interval(Inf, NaN)
+#     @test isnan(a.lo) && isnan(a.hi)
+#
+# end



Why are these tests disallowed?

I'm not sure, I think they should be added back, thanks.

lbenet · 2017-06-22T18:53:16Z

benchmark/benchmarks.jl

-    for op in (exp, log, sin, tan)
-        @bench string(op) $(op)($a)
-    end
-end



Why is this erased?

Rebase problem, sorry.

lbenet · 2017-06-22T18:58:35Z

src/intervals/intervals.jl

+else
+    const validity_check = false
+end
+
 abstract type AbstractInterval{T} <: Real end


(This was not there when I began reviewing.)
My suggestion (see elsewhere) is that the default is to have the validity being checked.

coveralls · 2017-06-22T23:14:16Z

Coverage decreased (-0.1%) to 91.513% when pulling 07fbd81 on simplify_constructor into f4be4b6 on master.

coveralls · 2017-06-22T23:14:16Z

Coverage decreased (-0.1%) to 91.513% when pulling 07fbd81 on simplify_constructor into f4be4b6 on master.

coveralls · 2017-06-24T17:25:34Z

Coverage decreased (-0.1%) to 91.522% when pulling 539e047 on simplify_constructor into f4be4b6 on master.

coveralls · 2017-06-24T17:25:34Z

Coverage decreased (-0.1%) to 91.522% when pulling 539e047 on simplify_constructor into f4be4b6 on master.

lbenet · 2017-06-24T19:39:59Z

I think that docstrings are still missing.

I still do not understand the need for the ENV variable.

While we agree to not export Interval, why not AbstractInterval? I thought the idea of having AbstractInterval was to allow other (external?) types use the same functionality. I know it is not yet implemented that way, so currently I agree to not export it; I just want to write this somewhere to be aware of it.

dpsanders · 2017-06-24T20:13:09Z

The ENV variable is the only method I have found to turn on and off the validity checks, such that the code is compiled with no validity checks if validity_check is false.

…heck is used

…n that case

…erval and Interval

coveralls · 2017-06-24T20:42:14Z

Coverage increased (+0.0008%) to 91.633% when pulling 5c3012a on simplify_constructor into f4be4b6 on master.

coveralls · 2017-06-24T20:42:14Z

Coverage increased (+0.0008%) to 91.633% when pulling 5c3012a on simplify_constructor into f4be4b6 on master.

coveralls · 2017-06-24T20:57:19Z

Coverage decreased (-0.5%) to 91.131% when pulling d4628a8 on simplify_constructor into e17aeef on master.

coveralls · 2017-06-24T21:14:42Z

Coverage decreased (-0.5%) to 91.131% when pulling d4628a8 on simplify_constructor into e17aeef on master.

coveralls · 2017-06-24T21:29:16Z

Coverage decreased (-0.5%) to 91.131% when pulling d4628a8 on simplify_constructor into e17aeef on master.

coveralls · 2017-06-24T21:29:17Z

Coverage decreased (-0.5%) to 91.131% when pulling d4628a8 on simplify_constructor into e17aeef on master.

dpsanders · 2017-06-24T22:31:35Z

@lbenet It would be great if you could have another final look over this. I think I have addressed all the comments and I'm pretty happy with how it's looking.

coveralls · 2017-06-24T23:05:20Z

Coverage decreased (-0.4%) to 91.224% when pulling 42124cc on simplify_constructor into e17aeef on master.

lbenet · 2017-06-25T02:35:15Z

I've checked and everything seems consistent and nice. I'm actually ready to merge, but I'd like to add the following suggestions:

Merging this PR implies that we will have as the default behavior (no IA_VALID ENV variable, or IA_VALID being whatever except "true") that validity_check is false. The user a priori has to use interval, which always checks the validity of the construction of the interval (complying with the standard); that's really nice. Then, why should there be a key IA_VALID at all? My suggestion is to get rid completely of it and also of validity_check.
Following with the previous point, we can also get rid of the current inner constructor (since there is no validity_check), and change the current outer constructors to be methods of interval. In that way, if the user really wants to use the Interval constructor, it will have to be called as IntervalArithmetic.Interval{T}(a,b), with the appropriate T, which is nasty enough to be done frequently.

What do you think about this?

lbenet · 2017-06-25T02:35:47Z

A priori, I am in favor of merging this, and also incorporate my two suggestions above.

lbenet · 2017-06-25T02:42:12Z

Or should the above suggestions be submitted as a different PR?

dpsanders · 2017-06-25T17:19:41Z

I think it makes sense to have the possibility of using validity_check with every construction of an Interval, in case any more bugs in our library code are found.

It's not actually clear to me if unexporting Interval is a good idea, since that currently makes using downstream packages (IntervalRootFinding) more difficult; but probably we just need to update those in the right way.

lbenet · 2017-06-25T19:10:49Z

Merging, then.

Regarding your comments, if validity_check is true, then we are passing through is_valid_interval twice. I agree that we should update the rest of the packages so interval is used.

dpsanders force-pushed the simplify_constructor branch from b942bec to af98333 Compare May 1, 2017 23:19

dpsanders force-pushed the simplify_constructor branch from af98333 to e6ea24c Compare May 2, 2017 03:27

dpsanders force-pushed the simplify_constructor branch from e6ea24c to 1467ff6 Compare May 20, 2017 19:49

dpsanders force-pushed the simplify_constructor branch from 1467ff6 to a3cb1ba Compare May 28, 2017 12:27

dpsanders force-pushed the simplify_constructor branch from 2dd3a2e to 8952216 Compare June 8, 2017 11:35

dpsanders force-pushed the simplify_constructor branch from 8952216 to 05f4c8a Compare June 22, 2017 18:02

dpsanders requested a review from lbenet June 22, 2017 18:22

lbenet reviewed Jun 22, 2017

View reviewed changes

dpsanders added 10 commits June 24, 2017 13:31

Fix docstring

29729f6

Return to error-free rounding

c0614cf

Add standard midpoint method if no alpha given

16422ed

Remove inner constructor for Interval. Add checked interval function

db4d74d

Add boolean to add or remove validity check on Interval

37d22be

Re-add interval function

032642d

Change some Interval to interval in tests and @interval

c04bf67

More Interval->interval fixes

92d087b

Add interval(a)

3755f9b

Add sum benchmarks

c2d0355

dpsanders added 12 commits June 24, 2017 13:31

Fixup after rebase

62421ca

Add check of IA_VALID environment variable to determine if validity_c…

c459399

…heck is used

Change internal constructor back to allow different types

ecd7f59

Add test for Travis environment variable and enable validity checks i…

edca3bd

…n that case

Remove incorrect rounding mode

6ba41f1

Do not export Interval constructor

b78255a

Fix some constructor tests to use interval and make a..b use interval

6fee75e

Reexport AbstractInterval

dc84de5

Re-add elementary function benchmarks

2db536b

Add docs for interval and improve error message

ada3fe9

Reinstate and fix test for single NaN that now throws an error

237d334

Remove special casing for Travis; add tests of difference between int…

d4628a8

…erval and Interval

dpsanders force-pushed the simplify_constructor branch from 03d9795 to d4628a8 Compare June 24, 2017 20:32

Remove simple_mid

42124cc

lbenet merged commit 00bbdce into master Jun 25, 2017

dpsanders deleted the simplify_constructor branch August 2, 2017 19:39

		@@ -5,37 +5,34 @@

		## Interval type

		const validity_check = false

Make Interval constructor fast by not including checks #26

Make Interval constructor fast by not including checks #26

Conversation

dpsanders commented May 1, 2017 • edited Loading

codecov-io commented May 2, 2017 • edited Loading

Codecov Report

coveralls commented May 28, 2017 • edited Loading

coveralls commented May 28, 2017 • edited Loading

coveralls commented Jun 8, 2017

coveralls commented Jun 8, 2017

lbenet commented Jun 11, 2017

lbenet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbenet Jun 22, 2017 • edited by dpsanders Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbenet Jun 22, 2017 • edited Loading

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

Choose a reason for hiding this comment

coveralls commented Jun 22, 2017

coveralls commented Jun 22, 2017 • edited Loading

coveralls commented Jun 24, 2017 • edited Loading

coveralls commented Jun 24, 2017

lbenet commented Jun 24, 2017

dpsanders commented Jun 24, 2017

coveralls commented Jun 24, 2017

coveralls commented Jun 24, 2017 • edited Loading

coveralls commented Jun 24, 2017 • edited Loading

coveralls commented Jun 24, 2017 • edited Loading

coveralls commented Jun 24, 2017 • edited Loading

coveralls commented Jun 24, 2017 • edited Loading

dpsanders commented Jun 24, 2017

coveralls commented Jun 24, 2017 • edited Loading

lbenet commented Jun 25, 2017

lbenet commented Jun 25, 2017

lbenet commented Jun 25, 2017

dpsanders commented Jun 25, 2017

lbenet commented Jun 25, 2017

dpsanders commented May 1, 2017 •

edited

Loading

codecov-io commented May 2, 2017 •

edited

Loading

coveralls commented May 28, 2017 •

edited

Loading

coveralls commented May 28, 2017 •

edited

Loading

lbenet Jun 22, 2017 •

edited by dpsanders

Loading

lbenet Jun 22, 2017 •

edited

Loading

coveralls commented Jun 22, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading

coveralls commented Jun 24, 2017 •

edited

Loading