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

#405 - Change intersection of two lines to set operation #408

Merged
merged 6 commits into from
Jul 31, 2018

Conversation

schillic
Copy link
Member

Closes #405.

@schillic schillic requested a review from mforets July 26, 2018 18:59
@schillic schillic force-pushed the schillic/405 branch 2 times, most recently from dc9ef1e to 4122c24 Compare July 26, 2018 19:33
Copy link
Collaborator

@kpotomkin kpotomkin left a comment

Choose a reason for hiding this comment

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

Hi @schillic

  1. Could we instead of
        # results in LAPACKException or SingularException if parallel	  
	        return Singleton(a \ b)
    catch

put

        # results in LAPACKException or SingularException if parallel	  
	        return Singleton(a \ b)
catch y
           if isa(y, LAPACKException) || isa(y, SingularException)

?

@schillic
Copy link
Member Author

okay 👍

@schillic schillic force-pushed the schillic/405 branch 3 times, most recently from 4b2d416 to 6fafb1d Compare July 29, 2018 09:58
@schillic schillic added this to the Release v1.3.0 milestone Jul 29, 2018
Copy link
Member

@mforets mforets left a comment

Choose a reason for hiding this comment

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

there are 2 failing doctests in concrete_intersection.jl.

for the one in LazySet.jl, rebase with master will fix it.

points[n] = intersection(Line(P.constraints[n]),
Line(P.constraints[1]))
points[n] = element(intersection(Line(P.constraints[n]),
Line(P.constraints[1])))
Copy link
Member

Choose a reason for hiding this comment

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

... and (later) we can move one step further and add a convex_hull that deals with a list of singletons (points), thus not needing these element(..). i think it would make the code a bit cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, we could add an iterator type over Singletons that returns their vectors, say, Singleton2Vector, and pass Singleton2Vector(points) to convex_hull. However, if no convex hull shall be applied, we still need to unwrap them.

@schillic
Copy link
Member Author

there are 2 failing doctests in concrete_intersection.jl.

Very good!

@mforets
Copy link
Member

mforets commented Jul 31, 2018

... ouch, that was unintended.

@mforets
Copy link
Member

mforets commented Jul 31, 2018

can be undone with $ git reset --hard HEAD~1

@mforets
Copy link
Member

mforets commented Jul 31, 2018

i lost your newer commits, sorry 😱

@schillic
Copy link
Member Author

Hm? What did you do?

@mforets
Copy link
Member

mforets commented Jul 31, 2018

git push -f (-f for forced), i did this inadvertently, since i really wanted to pull your branch.

@schillic
Copy link
Member Author

So this was only the fix of the doctests, right?

@mforets
Copy link
Member

mforets commented Jul 31, 2018

yes

@mforets
Copy link
Member

mforets commented Jul 31, 2018

i mean locally you have some commits that are not online, right?

@schillic
Copy link
Member Author

Yep, I force-pushed again.

@schillic
Copy link
Member Author

schillic commented Jul 31, 2018

Can we remove the "Changes requested" review without the "Dismiss review" button?
EDIT: Nevermind, I just had to update the page...

@mforets
Copy link
Member

mforets commented Jul 31, 2018

in https://github.com/JuliaReach/LazySets.jl/pulls it now showsup as Approved.
i didnt click on dismiss Review. i think that i clicked on the Files Changed and then sent the (new) review.

@schillic schillic merged commit 7c66417 into master Jul 31, 2018
@schillic schillic deleted the schillic/405 branch July 31, 2018 18:18
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