Heterogeneous graph and new associated RL pipeline. #229

gostreap · 2022-05-10T20:34:39Z

The purpose of this pull request is to allow the implementation of the new graph convolution discussed in issue #225.

The major novelties are :

Introduction of a heterogeneous graph with 3 distinct node types for constraints, variables and values.
The modification of the tripartite graph to better manage VarViews:
VarView relations were formerly represented as an edge between 2 distinct variables (the parent and its child). With this PR, VarView relations are now represented as constraints of type ViewConstraint linked to these 2 variables. When constraint_type is set to true in chosen_features, and a problem containts VarView relations, it now encodes the type of the constraint with 4 new dimensions reflecting the exact nature of the VarView relation. Please refer to featurize function in heterogeneousstaterepresentation.jl for more information.
The implementation of the new convolutions as described in Refacto of the Graph Convolution #225.
- The initialization layer of the convolution : HeterogeneousGraphConvInit
- The heterogeneous version of GraphConv: HeterogeneousGraphConv.
The compatibility of all downstream neural networks (CPNNs) with this new heterogeneous pipeline: HeterogeneousCPNN, HeterogeneousFullFeaturedCPNN and HeterogeneousVariableOutputCPNN.
The resolution of major bugs in the pipeline:
- Fix of model emptying after the resolution of each instance
- Fix of value ordering in the graph construction
The implementation of a new mean aggregation in the convolutional layers.

Minor updates also concern:

The removal of symmetries in the generation of graphcoloring problems
The correction of errors in the generation of KEP and RB problems

…aPearl.jl into heterogeneous

…l into heterogeneous

…aPearl.jl into heterogeneous

…l into heterogeneous

…aPearl.jl into heterogeneous

3rdCore · 2022-06-01T20:41:03Z

src/RL/utils/geometricflux/graphconv.jl

+            ef=fgs.ef,
+            gf=fgs.gf
+        )
+    end


The Zigote.ignore() should not contain the code block :

return BatchedFeaturedGraph{Float32}( fgs.graph; nf=g.σ.(g.weight1 ⊠ X .+ g.weight2 ⊠ X ⊠ A .+ g.bias), ef=fgs.ef, gf=fgs.gf )

As long as there no operation computed, this is maybe not a problem, but I'd rather remove this block from the ignore() if it doesn't lead to an error

3rdCore · 2022-06-01T20:42:07Z

src/RL/utils/geometricflux/heterogeneousgraphconv.jl

+        return BatchedHeterogeneousFeaturedGraph{Float32}(
+            contovar,
+            valtovar,
+            g.σ.(g.weightsvar ⊠ vcat(X1, H1, H2 ⊠ contovarN, H3 ⊠ valtovarN) .+ g.biasvar),
+            g.σ.(g.weightscon ⊠ vcat(X2, H2, H1 ⊠ vartoconN) .+ g.biascon),
+            g.σ.(g.weightsval ⊠ vcat(X3, H3, H1 ⊠ vartovalN) .+ g.biasval),
+            fgs.gf
+        )
+    end


Same comment here. What do you think of this @gostreap ?

We should not forget to correct this before merging with master. As the function is not recursive, it is ok to return under Zygote.ignore() statement but the operations that are computed inside the return here should be copied outside thr Zygote.ignore() domain. We have solved the same issue on heterogeneousgraphtransformer.jl.

What causes the Zygote errors is the construction of the BatchedHeterogeneousFeaturedGraph. It would even be safer to do separate construction of the object under Zygote.ignore() and return outside it.

3rdCore · 2022-06-06T21:53:45Z

src/RL/representation/default/cp_layer/types.jl

@@ -81,7 +81,7 @@ struct CPLayerGraph <: LightGraphs.AbstractGraph{Int}
    """
    function CPLayerGraph(cpmodel::CPModel)
        variables = Set{AbstractVar}(values(cpmodel.variables))
-        valuesOfVariables = branchable_values(cpmodel)
+        valuesOfVariables = sort(branchable_values(cpmodel))


can you explain briefly what the issue was with the previous pipeline ?

In short, the ordering of the actions taken by the agent was not consistent with the ordering of value node features in the graph. As a result, the values embeddings in FullFeaturedCPNN didn't match the actions taken by the agent afterwards. But this fix doesn't impact other CPNNs.

louis-gautier

We just have to check that Zygote.ignore() statements in all convolutional layers are well positioned and we can merge this branch on master.

louis-gautier · 2022-06-07T13:36:59Z

src/CP/core/model.jl

@@ -257,6 +257,8 @@ Empty the CPModel.
 """
 function Base.empty!(model::CPModel)
    empty!(model.variables)
+    empty!(model.branchable_variables)
+    empty!(model.branchable)


These lines fix a major bug caused by the fact that the model was not properly emptied after the resolution of each instance in the training process.

Nice spot !

louis-gautier · 2022-06-07T13:39:57Z

src/CP/core/model.jl

+            if !isempty(x.children)
+                for child in x.children
+                    cardinality += length(child.domain)
+                end


We now take all variables, including VarViews into consideration in global_domain_cardinality

louis-gautier · 2022-06-07T13:44:58Z

src/RL/representation/default/cp_layer/types.jl

-        for (x1, x2) in variableConnections
-            v1, v2 = VariableVertex(x1), VariableVertex(x2)
-            add_edge!(fixedEdgesGraph, nodeToId[v1], nodeToId[v2])
-        end


Yes absolutely. They are now represented as simple constraints (of type ViewConstraint) between 2 variables.

louis-gautier · 2022-06-07T13:49:42Z

src/RL/representation/default/defaultstaterepresentation.jl

+                    else
+                        error("WARNING: Unknwon VarViewType: please implement DefaultFeaturization for this type!")
+                    end
+                end


If the problem we are trying to solve has ViewConstraints, in constraint_type, we encode them thanks to 4 new components, respectively: one-hot encoding of the fact the considered constraint is a ViewConstraint, multiplication coefficient if the ViewConstraint is of type IntVarViewMul or IntVarViewOpposite, offset coefficient if the if the ViewConstraint is of type IntVarViewOffset, and one-hot encoding of the fact the ViewConstraint is of type BoolVarViewNot.

louis-gautier · 2022-06-07T13:53:15Z

src/RL/representation/default/heterogeneoustrajectorystate.jl

+    @assert all(var .<= size(fg.varnf,2)) "The variable index is out of bound"
+    @assert all(reduce(vcat,val) .<= size(fg.valnf,2)) "One of the values index is out of bound"
+    @assert all(reduce(vcat,[[tuple[2] for tuple in countmap([c for c in vals])] for vals in val]) .== 1) "values array contains one element at least twice"


Should we remove them now as they haven't caused any error so far?

louis-gautier · 2022-06-07T13:57:25Z

src/datagen/coloring.jl

@@ -94,8 +95,8 @@ function fill_with_generator!(cpmodel::CPModel, gen::HomogenousGraphColoringGene

    # edge constraints
    for i in 1:n
-        for j in 1:n
-            if i != j && rand(rng) <= p
+        for j in (i+1):n


This helps to remove symmetries when generating graphcoloring instances.

Should we remove them now as they haven't caused any error so far?

As long as computing performance is not a true bottleneck, I'd prefer to keep them in place as long as SeaPearl won't be (at least currently) put into production even if they are computationally inefficient.

Or we can remove only the last one which is the most greedy.

Ok no problem!

test/datagen/coloring.jl

…l into heterogeneous

3rdCore · 2022-06-07T15:35:15Z

src/RL/utils/geometricflux/graphconv.jl

-            ef=fgs.ef,
-            gf=fgs.gf
-        )
+        sum = replace(reshape(mapslices(x -> sum(eachrow(x)), A, dims=[1, 2]), 1, :, size(A, 3)), 0=>1)    


What is the reason for adding a replace( M, 0=>1) ?

edit : This is to avoid any division by 0, in case a node has no neighbors, which can happen for value that belongs to no domain after pruning.

In the case where a node has no neighbor, the sum is zero, which will then cause problems in the division.

Putting a 1 in the division should not be a problem, because it will divide a vector composed only of 0. Any constant other than 1 would have done the job as well.

Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com>

…l into heterogeneous

louis-gautier

I think we are now ready to merge with master.

* Add MaximumIndependentSet Generator * Change NotEqual to SumLessThan * Fix test by introducing LigthGraphs. everywhere * Introduce test for maximum independent set * Fix testset for maximumindependentset * Update src/CP/constraints/algorithms/matching.jl Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com> * Update src/CP/constraints/alldifferent.jl Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com> * Add missing LightGraphs. prefix Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com> Co-authored-by: louis-gautier <louisgautier99@gmail.com>

src/CP/valueselection/learning/utils.jl

3rdCore · 2022-06-08T17:20:29Z

src/RL/representation/default/defaultstaterepresentation.jl

@@ -112,7 +111,11 @@ Update the StateRepesentation according to its Type and Featurization.
 """
 function update_representation!(sr::DefaultStateRepresentation, model::CPModel, x::AbstractIntVar)
    update_features!(sr, model)
+    ncon = sr.cplayergraph.numberOfConstraints
+    nvar = sr.cplayergraph.numberOfVariables
+    sr.possibleValuesIdx = map(v -> indexFromCpVertex(sr.cplayergraph, ValueVertex(v)), collect(x.domain))


Did we specifically test this change ?

I don't think we tested it. Maybe we should create an issue for each behavior we want to test, without postponing the merge?

3rdCore · 2022-06-08T17:31:42Z

src/RL/representation/default/cp_layer/accessors.jl

+function adjacency_matrices(cplayergraph::CPLayerGraph)
+    g = LightGraphs.Graph(cplayergraph) # Update the graph with the new information
+    nvar = cplayergraph.numberOfVariables
+    ncon = cplayergraph.numberOfConstraints
+    nval = cplayergraph.numberOfValues
+    contovar = zeros(ncon, nvar)
+    valtovar = zeros(nval, nvar)
+    for (i, node) in enumerate(cplayergraph.idToNode)
+        if isa(node, ConstraintVertex)
+            neighbors = LightGraphs.outneighbors(g, i)
+            for neighbor in neighbors
+                contovar[i, neighbor - ncon] = 1
+            end
+        elseif isa(node, ValueVertex)
+            neighbors = LightGraphs.outneighbors(g, i)
+            for neighbor in neighbors
+                valtovar[i - ncon - nvar, neighbor - ncon] = 1
+            end
+        end
+    end
+    return contovar, valtovar


Did we tested this ?

Yes, we have unit tests for this in test/heterogeneousstaterepresentation.jl.

Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com>

gostreap and others added 30 commits May 9, 2022 09:38

Create heterogeneous file

2acaa09

Add check_dimensions for hetereogeneous graph

5eaa1c0

First version of HeterogeneousGraphConv

ec06f83

Add heterogeneous batched featured graph

b3ba22f

Add HeterogeneousTrajectoryState

271e5fe

Created HeterogeneousStateRepresentation

e14f1d3

Merge branch 'heterogeneous' of https://github.com/corail-research/Se…

19b4761

…aPearl.jl into heterogeneous

Consistent order of types of nodes + naming adjustments

0de9c50

Vertex ordering consistency

31eaa7b

Adapted the conversion of DefaultTrajectoryState

fdcaa50

Add HeterogeneousGraphConvInit + Fix HeterogeneousGraphConv

c1bd8aa

Merge branch 'heterogeneous' of github.com:corail-research/SeaPearl.j…

5911621

…l into heterogeneous

Fixed syntax errors

ab000e6

include heterogeneous file

605ef98

Fix HeterogeneousStateRepresentation

9fcc38f

Fix adjacency_matrices

a719af4

fix HeterogeneousTrajectoryState from HeterogeneousStateRepresentation

0b91a56

Fix compilation warning and multiple include of same file

55801bd

New management of VarViews in tripartite graph

e6c3d8f

Merge branch 'heterogeneous' of https://github.com/corail-research/Se…

cbe0fce

…aPearl.jl into heterogeneous

Add HeterogeneousCPNN

5ba7f63

Merge branch 'heterogeneous' of github.com:corail-research/SeaPearl.j…

ad84934

…l into heterogeneous

New management of VarViews for heterogeneous graphs

a20e754

Merge branch 'heterogeneous' of https://github.com/corail-research/Se…

18af577

…aPearl.jl into heterogeneous

Fix compilation error

8375617

Fixed error for registering VarView constraints

ad69d01

Merge branch 'heterogeneous' of https://github.com/corail-research/Se…

b1fa79a

…aPearl.jl into heterogeneous

Fixed indexing error

69b1832

Move totalLength

7af8721

Fixed ViewConstraint

0f9c82a

3rdCore reviewed Jun 1, 2022

View reviewed changes

3rdCore and others added 4 commits June 1, 2022 15:35

fixed a little bug on LegacyGraphColoringGenerator

1f3fbe5

Enable batch processing for HeterogeneousVariableOutputCPNN

7bab3ef

Fixed major bug in the pipeline and associated testsets

bafc490

Fixed graphcoloring generator testset after symmetry removal

6f9b317

louis-gautier marked this pull request as ready for review June 6, 2022 21:34

3rdCore reviewed Jun 6, 2022

View reviewed changes

louis-gautier requested changes Jun 7, 2022

View reviewed changes

Fix graphcoloring generator testset after symmetry removal

015a195

3rdCore reviewed Jun 7, 2022

View reviewed changes

test/datagen/coloring.jl Outdated Show resolved Hide resolved

gostreap added 4 commits June 7, 2022 11:16

Rework heterogeneousGraphConv for meanPooling

6fe00a8

Merge branch 'heterogeneous' of github.com:corail-research/SeaPearl.j…

1e360d4

…l into heterogeneous

Fix non batched meanPooling

8a3c454

Rework GraphConv for meanPooling

b55025e

3rdCore reviewed Jun 7, 2022

View reviewed changes

louis-gautier and others added 7 commits June 7, 2022 11:38

Fix graphcoloring generator testset after symmetry removal

7425faa

Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com>

Fixed SyntaxError

de0ce93

Fix graphcoloring generator testset after symmetry removal

fbdf3d0

Add test for heterogeneousVariableOutputCPNN

3fbc670

Clean variableOutputCPNN and heterogeneousVariableOutputCPNN

baff180

added small iterator for verbose

a880f74

Merge branch 'heterogeneous' of github.com:corail-research/SeaPearl.j…

276065c

…l into heterogeneous

louis-gautier approved these changes Jun 8, 2022

View reviewed changes

gostreap and others added 3 commits June 8, 2022 11:23

fixed empty! function : add model.adhocInfo

5237d5a

fix isempty!

5d834b9

3rdCore reviewed Jun 8, 2022

View reviewed changes

gostreap and others added 2 commits June 8, 2022 14:09

Update src/CP/valueselection/learning/utils.jl

8d2c68c

Co-authored-by: Tom Marty <59280588+3rdCore@users.noreply.github.com>

Management of ViewConstraints in nb_involved_constraint_propagation

8564aa4

gostreap merged commit 61220b5 into master Jun 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heterogeneous graph and new associated RL pipeline. #229

Heterogeneous graph and new associated RL pipeline. #229

gostreap commented May 10, 2022 •

edited by louis-gautier

Loading

3rdCore Jun 1, 2022 •

edited

Loading

3rdCore Jun 1, 2022 •

edited

Loading

louis-gautier Jun 6, 2022

louis-gautier Jun 6, 2022

3rdCore Jun 6, 2022

louis-gautier Jun 6, 2022

louis-gautier left a comment

louis-gautier Jun 7, 2022

3rdCore Jun 7, 2022

louis-gautier Jun 7, 2022

louis-gautier Jun 7, 2022

louis-gautier Jun 7, 2022

louis-gautier Jun 7, 2022

louis-gautier Jun 7, 2022

3rdCore Jun 7, 2022

louis-gautier Jun 7, 2022

3rdCore Jun 7, 2022

3rdCore Jun 7, 2022

gostreap Jun 7, 2022

louis-gautier left a comment

3rdCore Jun 8, 2022

gostreap Jun 8, 2022

3rdCore Jun 8, 2022

louis-gautier Jun 9, 2022

Heterogeneous graph and new associated RL pipeline. #229

Heterogeneous graph and new associated RL pipeline. #229

Conversation

gostreap commented May 10, 2022 • edited by louis-gautier Loading

3rdCore Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

3rdCore Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louis-gautier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louis-gautier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gostreap commented May 10, 2022 •

edited by louis-gautier

Loading

3rdCore Jun 1, 2022 •

edited

Loading

3rdCore Jun 1, 2022 •

edited

Loading