fix: Restrict the unnatural-partition skip to classical contractions

[chenpeizhi]

Depends on DrudgeCAS/fbitset#6. The deps/fbitset pin here points at that branch, because the new test adds a second translation unit and the header does not link twice without it. The pin should move back to master once that merges.

What is wrong

Parenther::opt skips a candidate when the broken summations shatter the factors into more than two chunks whose subproblems are all already memoized:

// Unnatural partition.
if (if_for_opt && chunks.size() > 2
    && std::all_of(chunks.cbegin(), chunks.cend(), [&mem](const Subset& i) {
           return mem.count(i.factors) != 0;
       })) {
    bsums_it.incr(false);
    continue;
}

This is an acceleration for classical tensor contractions, where every summation is involved by exactly two factors. The thesis says so directly, in the discussion of factor bipartitions in Chapter 5:

We also have developed code to accelerate the process specifically for classical TC problems where each summation index appears exactly twice. However, its acceleration to the overall problem is very limited. Hence it is omitted here.

The code applies it to every problem. When a summation is involved by more than two factors, it fails in two ways.

The optimal parenthesization is lost

Over 500 randomly generated contraction problems, the normal mode returned a result more expensive than the exhaustive mode for 68 of them. Never cheaper, always more expensive. Every one of the 68 has a summation involved by more than two factors. Not one classical problem is affected.

Since the normal mode is documented as finding the global minimum, and gristmill exposes it as ContrStrat.OPT, this is a silently wrong answer.

Every candidate can be skipped, and then the search crashes

If the skip consumes every candidate, the subproblem ends with no evaluation recorded. assert(!evals.empty()) at the end of opt then fires in a debug build. In a release build NDEBUG removes the assert and evals.front() reads an empty vector, which crashes.

The smallest case is a single summation over four factors:

// s = sum_i x[i] y[i] z[i] w[i]
std::vector<size_t> dims = { 100 };
std::vector<std::vector<size_t>> factors = { { 0 }, { 0 }, { 0 }, { 0 } };
Parenther<size_t> p(dims.cbegin(), dims.cend(), 1, factors.cbegin(), factors.cend());
p.opt(Mode::NORMAL, false);   // assertion failure, or a segfault under NDEBUG

Through gristmill this is a hard crash of the interpreter, with no exception and no traceback:

targets = [dr.define_einst(s, x[a] * y[a] * z[a] * w[a])]
optimize(targets, contr_strat=ContrStrat.OPT)      # segmentation fault
optimize(targets, contr_strat=ContrStrat.GREEDY)   # segmentation fault

Roughly five per cent of randomly generated configurations crash this way. TRAV, the default strategy, and EXHAUST are unaffected, because the skip is only active when mode != EXHAUST && !if_incl.

The fix

Gate the skip on the problem being classical, which is the assumption it was written under, and additionally never let it consume the last remaining candidate, so evals.front() can no longer be read with nothing found.

Verification

Every intermediate, its parenthesization and its cost were fingerprinted across 500 randomly generated problems in all six mode and inclusivity combinations, for four builds: before the C++20 migration, after it, with this fix, and with the skip removed entirely.

build	crashes	normal mode disagrees with exhaustive
before the C++20 migration	139	49 of 907, all non-classical
current master	139	49 of 907, all non-classical
this fix	0	0 of 1000
skip removed entirely	0	0 of 1000

On classical problems, all 1042 completed configurations are unchanged by this fix, so the acceleration is kept exactly where it is sound. This fix and removing the skip entirely agree on 2996 of 3000 configurations.

The thesis's own remark that the acceleration is "very limited" would also support removing it outright, which is simpler. That is left as a separate decision, since gating it is the change that provably alters nothing on the problems it was written for.

Tests

test/nonclassical.cpp covers both failures: the four-factor summation that used to leave no evaluation, and a problem where the optimal parenthesization is only reachable through a partition that used to be skipped. Both fail on master, by abort and by a cost mismatch respectively, and pass with the fix.

[Copilot]

Pull request overview

This pull request fixes incorrect skipping logic in Parenther::opt by restricting the “unnatural-partition” skip optimization to classical tensor contractions (each summation index appears in exactly two factors) and ensuring the skip cannot eliminate all candidates, preventing both suboptimal results and potential crashes.

Changes:

• Compute and store a if_classical_ flag during Parenther construction based on summation participation.
• Gate the “unnatural partition” skip on if_classical_ and require at least one evaluation to already exist before skipping, preventing empty-evaluation crashes.
• Add regression tests for non-classical contractions and wire them into the test build.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
include/libparenth.hpp	Adds if_classical_ detection and restricts the skip optimization to classical contractions while preventing “skip all candidates” cases.
test/nonclassical.cpp	Adds Catch2 tests covering non-classical contractions: previously-crashing case and previously-missed optimality case.
test/CMakeLists.txt	Adds the new nonclassical.cpp test translation unit to the test executable.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chenpeizhi]

Re-pointed deps/fbitset at master in 571b552, now that DrudgeCAS/fbitset#6 is in.

That merge was a rebase, so the branch commit this used to pin is not on master and would have dangled once the branch is deleted. The pin is now 0a21596, fbitset master, whose header content is identical to what was pinned before.

Tests pass in both the debug and the release build. This PR no longer depends on anything.