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

hull for IntervalBoxes #91

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
3 participants
@AKS1996
Copy link

commented Dec 15, 2017

analogous to hull for intervals

@dpsanders

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

Thanks very much for the PR! This is definitely functionality that we need.
I will add some comments on the code.

You will also need to add some tests.

@dpsanders
Copy link
Member

left a comment

Thanks for the PR!
I have added some comments that are hopefully useful. Please let me know if you have any questions.

@@ -105,3 +105,25 @@ end
#
# return results_list
# end

import IntervalArithmetic.hull

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

You don't need this, since we're still in the IntervalArithmetic module. You're just adding another method to the hull function.

hull(a, b)
Returns the `box hull` of the IntervalBoxes `a` and `b`, considered as
(extended) $N$ dimensional sets of real numbers, i.e. the smallest region that contains

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

I would say "the smallest IntervalBox that contains".

doc"""
hull(a, b)
Returns the `box hull` of the IntervalBoxes `a` and `b`, considered as

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

Backticks (`) are used for code, so IntervalBox should have backticks. "Box hull" could have normal quotes.

global_hi = convert(T,0)

for dimension in 1:N
global_lo = min(Base.getindex(A,dimension).lo,Base.getindex(B,dimension).lo)

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

Note that Base.getindex(A, dimension) can (and should) be written as just A[dimension].

global_lo = convert(T,0)
global_hi = convert(T,0)

for dimension in 1:N

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

In this loop, you are calculating the hull of each pair of intervals, so it can be just written as hull(A[i], B[i]). I would use i or n instead of dimension for the loop variable just for readability.

all of `a` and `b`.
"""
function hull(A::IntervalBox{N,T}, B::IntervalBox{N,T}) where {N,T}
result_list = Vector{Interval{T}}()

This comment has been minimized.

Copy link
@dpsanders

dpsanders Dec 15, 2017

Member

Here you are creating an intermediate array that it turns out is actually unnecessary. There is a function ntuple that makes a tuple out of a function that you can use here. This is used in a few places in the IntervalBox code.

@coveralls

This comment has been minimized.

Copy link

commented Dec 15, 2017

Coverage Status

Coverage decreased (-0.8%) to 91.198% when pulling f317ce5 on AKS1996:master into d442c3f on JuliaIntervals:master.

@dpsanders

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

Also, the setdiff.jl file was really meant for the setdiff functionality.
Maybe this should be moved to intervalbox.jl? Or maybe that file should now be split up more.

@dpsanders

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

I have just looked at the code again, and I think this functionality actually already exists, just written as . Could you please confirm that?

Surely the documentation could be improved on this point, though!

@dpsanders

This comment has been minimized.

Copy link
Member

commented Dec 15, 2017

On this line:

∪(X::IntervalBox{N,T}, Y::IntervalBox{N,T}) where {N,T} =

@AKS1996

This comment has been minimized.

Copy link
Author

commented Dec 16, 2017

True. The union U does the same thing

@AKS1996

This comment has been minimized.

Copy link
Author

commented Dec 16, 2017

But should U, mathematically, not computationally concerned, return a disjoint set of IntervalBoxes not conatining the extra space as in the hull
untitled

@dpsanders

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

Mathematically yes, I agree. This can be achieved using the setdiff function.

On the other hand, it is not clear why you would want this, computationally speaking - what would it be useful for? (I'm not saying that it isn't useful, I'm just not sure what the context would be.)

@AKS1996

This comment has been minimized.

Copy link
Author

commented Dec 16, 2017

I don't know it's use. Just took my interest

@AKS1996 AKS1996 closed this Dec 17, 2017

@AKS1996 AKS1996 force-pushed the AKS1996:master branch from f317ce5 to d442c3f Dec 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.