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.

Sign up for GitHub

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

Jump to bottom

issue 1193: Add bridge to that converts upper/lower constraints to interval constraints #1195

Merged
merged 2 commits into from Nov 27, 2020
Merged

issue 1193: Add bridge to that converts upper/lower constraints to interval constraints #1195

merged 2 commits into from Nov 27, 2020

Conversation

shadiakiki1986
Copy link
Contributor

@shadiakiki1986 shadiakiki1986 commented Nov 10, 2020 •
edited

Prompted by #1193

Used blegat's comment in the issue as well as gitter chat on November 6, 2020 8:59 PM

@shadiakiki1986 shadiakiki1986 mentioned this pull request Nov 10, 2020
Closed
blegat
blegat reviewed Nov 10, 2020
View reviewed changes
Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Looks real good, some minor comments :)

src/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
src/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
odow
odow reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

What happens if someone adds a GreaterThan and a LessThan bound constraint?

@blegat
Copy link
Member

blegat commented Nov 11, 2020

You would get an error as you cannot add an interval variable bound twice. Solver are recommended to still support variable GreaterThan and LessThan for now because of this

@shadiakiki1986 shadiakiki1986 force-pushed the sa/issue1193_bridge_upper_lower_to_interval branch 6 times, most recently from 54bf6f3 to 5e56bc7 Compare November 11, 2020 12:23
@shadiakiki1986 shadiakiki1986 marked this pull request as ready for review November 11, 2020 12:24
shadiakiki1986
shadiakiki1986 commented Nov 11, 2020
View reviewed changes
Copy link
Contributor Author

@shadiakiki1986 shadiakiki1986 left a comment

Choose a reason for hiding this comment

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

@blegat PR ready for your review. About the comment by @odow , should I add a test for it?

src/Bridges/Constraint/set_map.jl Outdated Show resolved Hide resolved
test/Bridges/Constraint/ltgt_to_interval.jl Outdated Show resolved Hide resolved
test/Bridges/lazy_bridge_optimizer.jl Show resolved Hide resolved
@shadiakiki1986 shadiakiki1986 force-pushed the sa/issue1193_bridge_upper_lower_to_interval branch from 5e56bc7 to 5e0e937 Compare November 16, 2020 11:02
@shadiakiki1986 shadiakiki1986 force-pushed the sa/issue1193_bridge_upper_lower_to_interval branch 2 times, most recently from 02a29ca to 15dc871 Compare November 16, 2020 15:50
@shadiakiki1986 shadiakiki1986 force-pushed the sa/issue1193_bridge_upper_lower_to_interval branch from 15dc871 to 2d6e52d Compare November 18, 2020 10:54
@shadiakiki1986
Copy link
Contributor Author

Except for codecov, PR is green

@blegat
Copy link
Member

blegat commented Nov 27, 2020

There was some conflicts with #1197 that I just resolved, we can merge when CI passes.

@shadiakiki1986
Copy link
Contributor Author

@blegat CI green. codecov/patch failed though.

blegat
blegat approved these changes Nov 27, 2020
View reviewed changes
Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Looks good thanks! Coverage sometimes miss lines of trivial functions that are inlined.

@blegat blegat merged commit 6f16730 into jump-dev:master Nov 27, 2020
@blegat blegat added this to the v0.9.19 milestone Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odow odow odow left review comments

@blegat blegat blegat approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
v0.9.19
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shadiakiki1986 @blegat @odow