Feature/alldifferent #115
Merged
Feature/alldifferent #115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
has an inconsistant behavior been introduced recently in the tsptw.jl test? Because it ran smoothly until I merged the last version of |
This reverts commit 7562f93.
| - `start::Int`: the free value node to start the search from. | ||
| - `free::Vector{Int}`: the list of all the free variables indexes. | ||
| """ | ||
| function augmentmatching!(digraph::DiGraph{Int}, start::Int, free::Set{Int})::Union{Nothing, Pair{Int, Int}} |
There was a problem hiding this comment.
I thing the function names and the variable names should be in camel case.
Example : augmentMatching
| """ | ||
| function augmentmatching!(digraph::DiGraph{Int}, start::Int, free::Set{Int})::Union{Nothing, Pair{Int, Int}} | ||
| parents = bfs_parents(digraph, start; dir=:out) | ||
| reached = intersect(free, findall(v -> v > 0, parents)) |
|
|
||
| Compute a random matching in a bipartite graph, between 1:lastfirst and (lastfirst + 1):nv(graph). | ||
| """ | ||
| function randommatching(graph::Graph{Int}, lastfirst::Int)::Matching{Int} |
| Compute a random matching in a bipartite graph, between 1:lastfirst and (lastfirst + 1):nv(graph). | ||
| """ | ||
| function randommatching(graph::Graph{Int}, lastfirst::Int)::Matching{Int} | ||
| seen = Set{Int}() |
| (lastfirst + 1):nv(digraph). A variable is assigned to a value if the directed | ||
| edge (Var => Val) exists. | ||
| """ | ||
| function matchingfromdigraph(digraph::DiGraph{Int}, lastfirst::Int)::Matching{Int} |
There was a problem hiding this comment.
matchingFromDigraph?
| the first group. | ||
| """ | ||
| function maximizematching!(digraph::DiGraph{Int}, lastfirst::Int)::Matching{Int} | ||
| currentmatching = matchingfromdigraph(digraph, lastfirst) |
There was a problem hiding this comment.
currentMatching
maximizeMatching
| function maximizematching!(digraph::DiGraph{Int}, lastfirst::Int)::Matching{Int} | ||
| currentmatching = matchingfromdigraph(digraph, lastfirst) | ||
| stop = currentmatching.size == lastfirst | ||
| freevar = Set(filter(v -> outdegree(digraph, v) == 0, 1:lastfirst)) |
There was a problem hiding this comment.
Use complet name
freeVariables
freeValues
It is longer to write, but clearer to read.
src/CP/constraints/alldifferent.jl
Outdated
|
|
||
| Return the node index of a value. | ||
| """ | ||
| function val2node(con::AllDifferent, val::Int) |
There was a problem hiding this comment.
I am not sure about the names. i Would use To instead of 2.
src/CP/constraints/alldifferent.jl
Outdated
| Return all the edges in a strongly connected component vars ∪ vars. | ||
| """ | ||
| function getalledges(digraph::DiGraph{Int}, vars::Vector{Int}, vals::Vector{Int})::Set{Edge{Int}} | ||
| res = Set{Edge{Int}}() |
src/CP/constraints/alldifferent.jl
Outdated
| end | ||
| end | ||
|
|
||
| allvar = 1:constraint.numberOfVars |
kimriouxparadis
approved these changes
May 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is my proposition for the AllDifferent constraint, relating to the issue #49. It contains:
My implementation uses only
LightGraphs.jland no third-party solver.The downside is that I manually implemented the matching algorithm and thus introduced many potential inefficiency sources. I tried to use
LightGraphsMatching.jlbut the BlossomV modules breaks the cross-platform portability as it requires some C++ files.Moreover the matching algorithm code is placed in the same file as the AllDifferent constraint as no dedicated folder exists for backend algorithms currently. But it could be wiser and cleaner to move it to a dedicated part of the architecture if more complex constraints are to be added in the future.
Thus potential improvements are:
One hesitation I had is in the last 2 test: I created a n-queens model to grant a full test of the constraint. But these tests thus rely on many other parts of the code. Is this anti-pattern? Because otherwise it is hard to guarantee the correct behavior of the constraint (for instance, I missed a few solutions in the n-queens at some point, but solving the problem integrally was the only way to detect this bug).