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

Basic support for multidimensional IntervalBox's #88

Merged
merged 14 commits into from
Mar 18, 2016
Merged

Basic support for multidimensional IntervalBox's #88

merged 14 commits into from
Mar 18, 2016

Conversation

dpsanders
Copy link
Member

FixedSizeArrays.jl makes this really easy.

@dpsanders
Copy link
Member Author

@lbenet I would like to merge this and tag a new version. Travis on 0.5 is not working for the moment. Is this OK with you?

@lbenet
Copy link
Member

lbenet commented Mar 15, 2016

Sorry, I've been too busy lately. Give me today to take a quick look on this, and if i don't react, just go ahead and merge.

@@ -1,5 +1,4 @@
julia 0.4
FactCheck 0.3
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need this here, to point out that the package uses this? I see that it appears in test/REQUIRE, but isn't it necessary here to install it as a dependency when using Pkg.add("ValidatedNumerics")?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it will be installed automatically when Pkg.test is run, and then deleted again. This is the recommended method for test-only dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for commenting on this. I didn't know!

@lbenet
Copy link
Member

lbenet commented Mar 15, 2016

LGTM. Aside from the comments, which are minor things, I think we should wait to tag the new version to include some of the things related to #91 (though perhaps not as in #92).

@dpsanders
Copy link
Member Author

I think this is ready.

@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

There are failing tests (julia 0.4); it actually didn't get to the ITF1788 test suite.

I'm checking this locally.

@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

They are also failing locally (for both 0.4.4 and 0.5-dev), precisely on "Operations with boxes".

import Base:
⊆, ∩, isempty

export IntervalBox
Copy link
Member

Choose a reason for hiding this comment

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

IntervalBox is exported in src/ValidatedNumerics.jl; I don't think it needs to be exported here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@dpsanders
Copy link
Member Author

Sorry, should be OK now.

@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

Tests for Julia 0.4 are passing; so I will merge (in some 15 minutes). I want to see if the tests in 0.5 will finish or not.

@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

I noticed that this PR cannot by merged due to a conflict. Can you solve it?
(Tests in travis 0.5 are not passing, with the same all problem...)

@dpsanders
Copy link
Member Author

Finally ready.

@dpsanders
Copy link
Member Author

Maybe I should squash?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.08%) to 95.099% when pulling 572e3d5 on multidim into bf62bf4 on master.

@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

Let's leave this as it is. I'm merging!

lbenet added a commit that referenced this pull request Mar 18, 2016
Basic support for multidimensional IntervalBox's
@lbenet lbenet merged commit 90d5e6f into master Mar 18, 2016
@lbenet
Copy link
Member

lbenet commented Mar 18, 2016

It's really a nice PR

This was referenced Mar 19, 2016
@dpsanders dpsanders deleted the multidim branch March 20, 2016 22:38
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.

3 participants