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

Add decorated intervals #112

Merged
merged 26 commits into from
Apr 27, 2016
Merged

Add decorated intervals #112

merged 26 commits into from
Apr 27, 2016

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Apr 25, 2016

This PR implements the decoration system, and it is tested against the ITF1788 suite. (The tests with Julia 0.5 were hanging locally on my machine.) I needed to rebase from the branch "decorations" to include a bunch of changes (since version 0.2). Still missing is to extend our own tests. I think this PR is ready for reviewing and corrections.

@lbenet
Copy link
Member Author

lbenet commented Apr 25, 2016

Also, the documentation is missing...

@dpsanders dpsanders mentioned this pull request Apr 25, 2016
4 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.03%) to 93.45% when pulling efce8d0 on decorations_rebased into a64e948 on master.

@dpsanders
Copy link
Member

Great work, thanks! Let me look through it carefully.

@lbenet
Copy link
Member Author

lbenet commented Apr 25, 2016

As you'll see, it is heavily based on yours, extending it whenever was needed.

@dpsanders
Copy link
Member

It's pretty amazing that the ITF1788 tests pass! Really nice work extending what I had done.

Since all the tests pass, maybe we should just merge this and then iterate on it? Or what else do you think needs to be done?


for f in other_functions
@eval $(f){T}(xx::DecoratedInterval{T}, yy::DecoratedInterval{T}) =
DecoratedInterval($(f)(interval(xx), interval(yy)), trv)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think just putting trv is correct. Shouldn't it be the decay of the others? Maybe not for hull (which is sort of a discontinous operation), but for the others (which seem to be in some sense "continuous" operations). Or is this discussed in the Standard?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is discussed in the standard; there, it is recommended that the user adapts it by changing the decoration. I'll add this (and other) docstrings explaining this.

@codecov-io
Copy link

Current coverage is 93.43%

Merging #112 into master will increase coverage by -1.04%

@@           master       #112   diff @@
========================================
  Files          16         18     +2   
  Lines         689        853   +164   
  Methods         0          0          
  Branches        0          0          
========================================
+ Hits          651        797   +146   
- Misses         38         56    +18   
  Partials        0          0          
  1. 2 files in src were modified. more

Sunburst

Powered by Codecov. Last updated by 978eb8f

@coveralls
Copy link

coveralls commented Apr 26, 2016

Coverage Status

Coverage decreased (-1.05%) to 93.435% when pulling cddf515 on decorations_rebased into a64e948 on master.


function DecoratedInterval(I::Interval, d::DECORATION)
dd = decoration(I)
Int(dd) < 2 && return new(I, dd)
Copy link
Member

Choose a reason for hiding this comment

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

Use symbolic values here, not numeric: dd <= trv

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'll implement it as you suggest.

@lbenet
Copy link
Member Author

lbenet commented Apr 27, 2016

I've just pushed a new commit following your suggestion; it includes some docstrings. (I hope to push a new one with more docstrings; just give me a few days.)

@lbenet
Copy link
Member Author

lbenet commented Apr 27, 2016

Incidentally, can you take a look on @decorated; I don't think it works...

@lbenet
Copy link
Member Author

lbenet commented Apr 27, 2016

Also, other tests should be added, covering decorated intervals with extended precision.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage decreased (-1.03%) to 93.458% when pulling 0a4ef2f on decorations_rebased into a64e948 on master.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage decreased (-1.03%) to 93.458% when pulling fee1b72 on decorations_rebased into a64e948 on master.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage decreased (-1.03%) to 93.458% when pulling d073a37 on decorations_rebased into a64e948 on master.

@dpsanders
Copy link
Member

I think we should merge this now.

@lbenet
Copy link
Member Author

lbenet commented Apr 27, 2016

Fine with me.

@dpsanders dpsanders force-pushed the decorations_rebased branch from d073a37 to d0707a8 Compare April 27, 2016 22:43
@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.2%) to 92.991% when pulling d0707a8 on decorations_rebased into 24b7e2f on master.

@codecov-io
Copy link

codecov-io commented Apr 27, 2016

Current coverage is 92.99%

Merging #112 into master will decrease coverage by -1.46%

@@             master       #112   diff @@
==========================================
  Files            15         18     +3   
  Lines           685        856   +171   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            647        796   +149   
- Misses           38         60    +22   
  Partials          0          0          
  1. 2 files in src/intervals were modified. more
  2. 1 files in src were modified. more

Powered by Codecov. Last updated by 132a93b

@dpsanders dpsanders changed the title WIP: Decorations Add decorated intervals Apr 27, 2016
@dpsanders dpsanders merged commit 0c6464c into master Apr 27, 2016
@dpsanders dpsanders deleted the decorations_rebased branch April 27, 2016 22:57
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.

4 participants