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

[Bridges] Fix ConstraintIndex conflicts between variable and constraint bridges #2362

Merged
merged 22 commits into from
Dec 21, 2023
Merged

[Bridges] Fix ConstraintIndex conflicts between variable and constraint bridges #2362

merged 22 commits into from
Dec 21, 2023

Conversation

blegat
Copy link
Member

@blegat blegat commented Dec 14, 2023 •
edited
Loading

Closes #2358

@blegat blegat mentioned this pull request Dec 14, 2023
Merged
13 tasks
@odow
Copy link
Member

odow commented Dec 18, 2023

Let me know when you get this to a point that you want me to take a look.

@blegat
Copy link
Member Author

blegat commented Dec 19, 2023

Ready for review

odow
odow reviewed Dec 19, 2023
View reviewed changes
src/Bridges/Variable/map.jl Outdated Show resolved Hide resolved
odow
odow reviewed Dec 19, 2023
View reviewed changes
src/Bridges/Variable/map.jl Outdated Show resolved Hide resolved
odow
odow reviewed Dec 19, 2023
View reviewed changes
src/Bridges/Variable/map.jl Outdated Show resolved Hide resolved
odow
odow reviewed Dec 19, 2023
View reviewed changes
Copy link
Member

@odow odow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this needs far more tests. I always get a bit nervous with these changes.

https://github.com/jump-dev/MathOptInterface.jl/actions/runs/7268307681

here's master
https://github.com/jump-dev/MathOptInterface.jl/actions/runs/7269105628

@odow odow mentioned this pull request Dec 19, 2023
Closed
3 tasks
@odow
Copy link
Member

odow commented Dec 20, 2023

This looks like it broke a bunch of solver tests, including Gurobi and CPLEX. So as well as the fix, we obviously need more tests in MOI proper.

@blegat blegat force-pushed the bl/ensure_available branch from 84e422e to 947b8d9 Compare December 20, 2023 09:10
@blegat
Copy link
Member Author

blegat commented Dec 20, 2023

we obviously need more tests in MOI proper.

Yes, that's also the motivation for #2357

@blegat
Copy link
Member Author

blegat commented Dec 20, 2023

New run: https://github.com/jump-dev/MathOptInterface.jl/actions/runs/7273606634

@blegat
Copy link
Member Author

blegat commented Dec 20, 2023

New run: https://github.com/jump-dev/MathOptInterface.jl/actions/runs/7277998597

@odow
Copy link
Member

odow commented Dec 20, 2023

I don't really have an opinion on the design. But I also don't have a suggestion for anything better.

I do think we need 100% coverage before merging though.

@blegat blegat force-pushed the bl/ensure_available branch from a42fc0c to 38ab2ac Compare December 21, 2023 09:17
@blegat
Copy link
Member Author

blegat commented Dec 21, 2023

I do think we need 100% coverage before merging though.

We do now

odow
odow approved these changes Dec 21, 2023
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.

I'll merge so we can re-trigger #2357.

I'll also pay careful attention to solver-tests before tagging a new MOI release.

@odow odow merged commit add2803 into master Dec 21, 2023
17 checks passed
@odow odow deleted the bl/ensure_available branch December 21, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odow odow odow approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blegat @odow