Skip to content

Feat/gcc - #1250

Merged
cprudhom merged 5 commits into
developfrom
feat/gcc
Oct 1, 2026
Merged

cprudhom merged 5 commits into
developfrom
feat/gcc

Conversation

@cprudhom

Copy link
Copy Markdown
Member

Porting of the Global Cardinality BC and AC filtering algorithms from choco-1.2.0.6.
The primary aim was educational, for the purposes of a Constraint Programming course on consistency.
Just to be on the safe side, I ran the tests on MZN instances (default, BC and AC) and, as the BC results are satisfactory, I propose that this configuration be set as the default.

Scatter plots

image image image

Cactus plots

image

Gains and losses

image

PropGccBC (bounds-consistency, Quimper et al. CP-2003) and PropGccAC
(arc-consistency, Regin AAAI-96) are posted alongside the existing
PropFastGCC, opt-in via GlobalCardinality(..., "BC"/"AC") or
model.globalCardinality(..., "BC"/"AC"). Both share a dense flow
network over the full [min, max] value range so that unrestricted
("escape") values are handled soundly.

Includes a fix for AlgoGccAC's augmenting-path search: after a warm
start where every variable is already matched, the search had no
genuinely free variable to end an augmenting path on, so a later
propagate() that raised several values' lower bounds at once could
wrongly report a value's minimum as unreachable even though a trivial
reassignment existed (found via fuzzing against a DEFAULT+search
ground truth, after testDeficitAppearingAfterWarmStart caught it on
real peaceable_queens instances under optimization). The search now
also accepts a variable sitting on a value with slack as a valid
endpoint, mirroring the existing donation-through-source mechanism.
Benchmarked across three independent campaigns (macpro, srv/Slurm ×2)
on real FlatZinc instances: BC matches AC on solution quality while
being algorithmically cheaper, and both markedly outperform the
previous DEFAULT (PropFastGCC-only) on tightly-constrained instances
-- sometimes by orders of magnitude in nodes explored.

Every caller of the 4-arg globalCardinality(...)/GlobalCardinality(...)
overload (no explicit consistency string) now gets BC. The choice is
centralized in GlobalCardinality.defaultConsistency() and overridable
via the choco.gcc.consistency system property, so it can be swapped
for benchmarking without touching calling code.
Modeler.modelGCC_AC/modelGCC_BC mirror the existing
modelAllDiffAC/modelAllDiffBC pattern: only the decision variables are
exposed to ConsistencyChecker, since PropGccAC/PropGccBC only
guarantee their consistency level on decision variables (cardinality
variables are left to PropFastGCC's weaker bound reasoning, see
PropGccAC's javadoc) -- testing cardinality domains the same way
would flag intentional, documented behavior as a false failure. The
restricted value set is fixed by the (call-invariant) variable count,
never derived from the incoming domains' actual content, since the
checker re-invokes the modeler with one variable narrowed to a single
value at a time and the constraint being tested must stay the same
constraint across those calls.

TestConsistency gains testGCC_AC/testGCC_BC; TestCorrectness's
existing testGCC() is extended to also check modelGCC_AC/modelGCC_BC
against the decomposition.
@cprudhom cprudhom added this to the 6.0.2 milestone Sep 29, 2026
@cprudhom cprudhom self-assigned this Sep 29, 2026
@mergify

mergify Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@cprudhom

Copy link
Copy Markdown
Member Author

I forgot to adapt the stats for mzn and xcsp3 test suite.

Switching GlobalCardinality's default consistency to BC changes the
search statistics (nodes/fails) of the test instances relying on GCC.
Solution counts and best values are unchanged.

@ArthurGodet ArthurGodet left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is not necessary and I woul dunderstand if it stays the same as currently, but since PropGccAc and PropGccBc share most of their code, it might be interesting to factorise their code into a PropGcc class.

Both propagators shared an identical propagate() (dense minOcc/maxOcc
construction), a near-identical constructor, isEntailed() and
toString(); the only real differences were the PropagatorPriority, the
filtering algorithm, and the propagation conditions.

Introduce a GccFilter interface implemented by AlgoGccBC/AlgoGccAC, and
collapse the two propagators into one PropGcc class parameterized by
GlobalCardinality.Consistency (BC/AC), which now decides the priority,
the algorithm, and getPropagationConditions from that single field.
@cprudhom
cprudhom merged commit 5aa8eec into develop Oct 1, 2026
22 checks passed
@cprudhom

cprudhom commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

GCC review follow-up (post-8db84b219)

Since merging PropGccBC/PropGccAC into a single PropGcc (8db84b219), we ran a point-by-point review of AlgoGccBC/AlgoGccAC against the legacy choco-1.2.06 implementation, plus two performance investigations. Summary:

  • BC review — all 5 points validated: index-based sorting, PartialSum (re)allocation, effective-maxOcc tightening (already handled by PropFastGCC, confirmed by reading its code), the BC/AC propagator merge, and zero-capacity-slot handling in the union-find structures. Nothing left untransposed from the legacy; no latent issue found.
  • AC review — all 4 points validated: non-backtrackable warm-started matching (same pattern as AlgoAllDiffAC), SCC via StrongConnectivityFinder/Tarjan (the legacy's double-DFS + dense transitive-closure matrix turned out to be dead code, never read), the augmenting-path BFS, and the dense value range (a genuine improvement — the legacy matching-based AC has no equivalent for unrestricted domain values at all).
  • Incremental domVars experiment — prototyped maintaining domVars via delta monitors instead of rebuilding it on every propagate(). Found and fixed a real bug along the way (incremental state must itself be backtrackable, or it goes stale across search backtracks). Benchmarked on 80 real FlatZinc instances: correctness held, but no measurable performance gain (slightly slower on average) — abandoned, code removed.
  • buildResidualDigraph() sweep cost — measured range vs. the number of restricted values on the same 80 instances: range is never meaningfully larger (at most +1), so trimming the unconditional O(n+range) sweep isn't worth prototyping.

No correctness issues remain open. The only unresolved item is a pure allocation-reuse question (reusing PartialSum/minOcc/maxOcc arrays across calls), which needs allocation profiling rather than a design call — deferred, optional.

@cprudhom
cprudhom deleted the feat/gcc branch October 1, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants