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

#1316 - Set difference between intervals #1330

Merged
merged 13 commits into from
May 6, 2019
Merged

#1316 - Set difference between intervals #1330

merged 13 commits into from
May 6, 2019

Conversation

mforets
Copy link
Member

@mforets mforets commented May 1, 2019

Closes #1316.

@mforets mforets requested a review from schillic May 2, 2019 01:53
Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

I have not looked at the code yet. First let us decide on the output type.

src/concrete_difference.jl Outdated Show resolved Hide resolved
src/concrete_difference.jl Outdated Show resolved Hide resolved
test/unit_Interval.jl Outdated Show resolved Hide resolved
@mforets mforets requested a review from schillic May 2, 2019 22:59
@schillic
Copy link
Member

schillic commented May 4, 2019

I am thinking about using - instead of \. The reason is that \ is already used for left division.

Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

I do not see how your function computes the difference. At least give an explanation in the docs, but I have the feeling that it is just wrong.

src/difference.jl Outdated Show resolved Hide resolved
src/difference.jl Show resolved Hide resolved
src/difference.jl Show resolved Hide resolved
@mforets
Copy link
Member Author

mforets commented May 4, 2019

I am thinking about using - instead of . The reason is that \ is already used for left division.

Thanks for raising the issue. Note that - is the difference between intervals in the arithmetic sense, so it would be a source of confusion if we use it in LazySets as alias for set diff (and it is already taken). Example:

julia> I1 = Interval(0, 1)
julia> I2 = Interval(2, 4)

julia> I1 - I2
Interval{Float64,IntervalArithmetic.Interval{Float64}}([-4, -1])

julia> I1.dat - I2.dat # the above implemented this rule
[-4, -1]

On the other hand, it is also true that backslash is used for division (although we don't offer this method in the Interval wrapper), example:

julia> I1.dat \ I2.dat
[2, ∞]

So i would suggest that we just keep difference for now. Unfortunately, set minus \ doesn't seem to be available as function name. Maybe there is another unicode alias?

@schillic
Copy link
Member

schillic commented May 5, 2019

Note that - is the difference between intervals in the arithmetic sense, so it would be a source of confusion if we use it in LazySets as alias for set diff (and it is already taken).

True! So let us keep \.

@mforets
Copy link
Member Author

mforets commented May 5, 2019

I do not see how your function computes the difference. At least give an explanation in the docs, but I have the feeling that it is just wrong.

I've added an Algorithm section. What do you think?

src/difference.jl Outdated Show resolved Hide resolved
src/difference.jl Outdated Show resolved Hide resolved
schillic and others added 2 commits May 5, 2019 18:52
@mforets
Copy link
Member Author

mforets commented May 5, 2019

Alright, thanks for your review. I'm adding some docs and moving isflat to Interval.jl before merging (we could have isflat for AbstractHyperrectangle too! (It has to check the radius_hyperrectangle is flat in any dimension..)

Keeping \ as an alias for difference then and adding extra docs with the potential overlap with set \ from IntervalArithmetic.Interval.

Copy link
Member

@schillic schillic left a comment

Choose a reason for hiding this comment

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

Consider squashing.

@mforets mforets merged commit 4f5d775 into master May 6, 2019
@mforets
Copy link
Member Author

mforets commented May 6, 2019

Squashed and merged, thanks a lot for the review!

@mforets mforets deleted the mforets/1316 branch May 6, 2019 07:51
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.

Set difference between intervals
2 participants