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

#964 - Check if polytope is bounded on construction #978

Merged
merged 5 commits into from
Jan 19, 2019
Merged

Conversation

schillic
Copy link
Member

@schillic schillic commented Jan 8, 2019

Closes #964.

@schillic schillic requested a review from mforets January 8, 2019 21:12
@mforets
Copy link
Member

mforets commented Jan 16, 2019

Here i have an API / aesthetic concern: i see no need to add a new method validate_boundedness, since we already have isbounded. We do need a new name for the keyword argument, which following other conventions throughout could be called check_boundedness.

@schillic
Copy link
Member Author

schillic commented Jan 16, 2019

We need it because isbounded(::HPolytope) does not perform any checks.

@mforets
Copy link
Member

mforets commented Jan 16, 2019

Ok. What about the keyword in the constructor? I slightly prefer check_boundedness.

src/HPolygon.jl Outdated Show resolved Hide resolved
src/HPolygon.jl Outdated Show resolved Hide resolved
@schillic schillic force-pushed the schillic/964 branch 2 times, most recently from 1343cd8 to 61bdb8f Compare January 16, 2019 21:26
@schillic schillic force-pushed the schillic/964 branch 2 times, most recently from 182b9e2 to 4b2a9e2 Compare January 18, 2019 19:39
@schillic
Copy link
Member Author

The last two commits are new. Can you check again?

@schillic schillic requested a review from mforets January 19, 2019 12:06
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.

lgtm

@schillic schillic merged commit dac4401 into master Jan 19, 2019
@schillic schillic deleted the schillic/964 branch January 19, 2019 12:34
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.

2 participants