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

Typo in sets #1024

Merged
merged 7 commits into from
Feb 14, 2020
Merged

Typo in sets #1024

merged 7 commits into from
Feb 14, 2020

Conversation

dourouc05
Copy link
Contributor

Needed part for jump-dev/JuMP.jl#2051, to gather some feedback.

src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
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.

We need to think about sets containing functions.

For example:

x in Count([u, v, w], 2)

is better as

[x, u, v, w] in Count(2)

@dourouc05
Copy link
Contributor Author

What do you think of the latest version?

src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
src/sets.jl Outdated Show resolved Hide resolved
@dourouc05
Copy link
Contributor Author

Better this way? I also added Sort, SortPermutation, BinPacking, and CapacitatedBinPacking. I think this set of constraints is enough as a first step.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

Merging #1024 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1024      +/-   ##
==========================================
- Coverage   95.19%   95.16%   -0.04%     
==========================================
  Files         100      100              
  Lines       11279    11283       +4     
==========================================
  Hits        10737    10737              
- Misses        542      546       +4
Impacted Files Coverage Δ
src/sets.jl 95.28% <ø> (ø) ⬆️
src/Bridges/Constraint/geomean.jl 95.23% <0%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b259fa...a645758. Read the comment docs.

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.

Hmm. I like the direction this is going, but there's a lot of sets here.

I propose that you (we) develop a ConstraintProgrammingExtensions.jl package to contain these definitions.

Then ConstraintSovler.jl can depend on that package, enabling:

using JuMP
using ConstraintProgrammingExtensions
using ConstraintSolver

const CP = ConstraintProgrammingExtensions

model = Model(ConstraintSolver.Optimizer)
@variable(model, x[1:4], Int)
@constraint(model, x in CP.AllDifferent(4))

It's similar to what I proposed for #996 (comment).

The upside of an extension package is we would get to play around with different sets and definitions without having to depend on them forever in MOI. If, at some point, they become stable and well supported, then we could look at bringing them into MOI.

@dourouc05
Copy link
Contributor Author

I completely agree with your objection.

Regarding the JuMP PR (jump-dev/JuMP.jl#2051), I suppose it makes no sense to have support in JuMP for sets that are not in MOI. This PR could then be reduced to the minimum to allow implementing things, with the actual constraints being implemented in that other package (say ConstraintProgrammingExtensions.jl, or another one so that solvers do not have a dependency on JuMP, like JuCP.jl or anything).

(Also tagging @blegat on this, due to his feedback on the JuMP PR.)

Also, would that ConstraintProgrammingExtensions.jl package be a good place to implement bridges, like in MOI?

@odow
Copy link
Member

odow commented Feb 12, 2020

You could even have JuMP as a dependency to ConstraintProgrammingExtensions, and export a macro like

@logicalconstraint(model, all_different(x))

which re-writes to

@constraint(model, x in CP.AllDifferent(length(x))

I guess the points I want to make are:

  1. If you want to work on constraint programming for JuMP, we want to help make that happen
  2. In the long-term, we're supportive of integrating constraint programming into JuMP
  3. In the near-term, I don't know if we've explored the space enough to understand what works and what doesn't. It'd be nice to understand what syntax decisions mini zinc made, why they made them, and what types of constraints they support.
  4. Developing a constraint programming package now with the view to bringing some of it into MOI and some of it into JuMP in the future seems like the way to go.

So I would close #2051, but more because the timing is off, rather than us not wanting to add it.

Also, would that ConstraintProgrammingExtensions.jl package be a good place to implement bridges, like in MOI?

Yes, perfect place for it. CP -> MIP bridges are probably a good way of getting solvers hooked up to test various things.

@blegat
Copy link
Member

blegat commented Feb 13, 2020

Developing a constraint programming package now with the view to bringing some of it into MOI and some of it into JuMP in the future seems like the way to go.

I agree, MOI already integrated 3 packages: MathOptFormat, MathOptInterfaceTests and MathOptInterfaceBridges.
It was easier to start with separate packages and experiment and once they matured merge them to MOI.
For instance, in MOI, we don't like making breaking releases hence we block any breaking changes. That might not be appropriate when you want to experiment and iterate with user feedback in the early stage.
It's also probably simpler if you make one package depending on both JuMP and MOI that defines sets, bridges and extension of JuMP macros.
Examples are PolyJuMP and SumOfSquares which defines new sets, new bridges and extend JuMP macros. In this case, we don't plan to merge them to MOI. First because they have a lot of dependencies in JuliaAlgebra, and second because MOI is designed such that there is no disadvantage of having extensions outside of MOI/JuMP. The goal is to have a distributed ecosystem to support the distributed and explorative nature of the world of research and academics :)

dourouc05 added a commit to JuliaConstraints/ConstraintProgrammingExtensions.jl that referenced this pull request Feb 13, 2020
@dourouc05 dourouc05 changed the title [WIP] Constraint-programming sets Typo in sets Feb 13, 2020
@dourouc05
Copy link
Contributor Author

Thanks for your thoughts! I just created a new package for these sets (not yet registered): https://github.com/dourouc05/ConstraintProgrammingExtensions.jl

I repurposed this PR just to correct two typos in IndicatorSet.

@@ -711,9 +711,9 @@ end
"""
IndicatorSet{A, S <: AbstractScalarSet}(set::S)

``\\{((y, x) \\in \\{0, 1\\} \\times \\mathbb{R}^n : y = 0 \\implies x \\in set\\}``
Copy link
Contributor

Choose a reason for hiding this comment

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

you added opening parentheses, which is the closing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I removed parentheses that were not closed anywhere.

@odow odow merged commit fb303dc into jump-dev:master Feb 14, 2020
@odow
Copy link
Member

odow commented Feb 14, 2020

Thanks @dourouc05! Let's keep discussing things in your extension package.

@blegat blegat added this to the v0.9.11 milestone Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants