Atlanta workshop test-suite overhaul #4322
Open
kellerlv wants to merge 280 commits into
Open
Conversation
mahrud
reviewed
May 18, 2026
Contributor
Author
There was a problem hiding this comment.
Got it, will take a look
mahrud
reviewed
May 18, 2026
Member
There was a problem hiding this comment.
Somewhat confused about these added tests. Are these things that weren't already checked by existing tests?
Contributor
Author
There was a problem hiding this comment.
Some of these functions were legitimately untested and we've just been going on the principle that any untested exported function should have some test, however trivial. But some of these are indeed indirectly tested, so they can easily be pruned down.
Member
|
@kellerlv or @tbrysiewicz, when this is ready, could one of you rebase this PR and force push to "linearize" the commit history? (i.e. get rid of merge commits) |
Contributor
Author
Yup, we can do that |
Revive the reversed-fence ///TEST block: a reversed delimiter made Macaulay2 parse it as inert text, and its stale golden-hash asserts referred to undefined variables. Replace it with a self-contained test of approximation and its Total and CoDepth options. Add TEST blocks for the previously untested export socleDegrees, for the setupRings options Characteristic and Randomize, and a regression test for freeResolution(..., LengthLimit => 0) over a hypersurface -- code the package text noted had once crashed M2 v8.2. Each new block has a header comment describing what it tests. Remove a stray scratch /// block from the documentation section, and make the socleDegrees documentation example actually call socleDegrees. check now runs 15 tests, up from 11, all passing; installPackage is clean.
Adds four TEST blocks to LieAlgebraRepresentations/documentation.m2 covering exported symbols that previously had no test coverage: the fraktur shorthand 𝔞-𝔤 for simple Lie algebras, the ω shorthand for fundamental weights, characterRing, LieAlgebraModuleFromWeights, and adams. The tests assert mathematical properties -- that LieAlgebraModuleFromWeights inverts weightDiagram, that the character of a module lives in the character ring, that Adams operations preserve dimension -- and include an error test for bad input.
Document every TEST block with a header comment describing what it checks, and remove an exact-duplicate test block. Add a TEST asserting the multiplication-table output of multTableOneOne and multTableOneTwo under the Labels and Compact options; previously those options were exercised only in documentation examples and never asserted, so a regression in the table formatting would go uncaught. Add an error-test block covering the input checks of resLengthThreeAlg, multTableOneOne, multTableOneTwo, and makeRes. check now runs 13 tests, all passing; installPackage is clean.
Adds four TEST blocks to TensorComplexes.m2 covering exported symbols that previously had no direct test coverage: traceMap, minorsMap, hyperdeterminantMatrix, pureResES1, the MonSize option, and the LabeledModuleMap type. The tests assert mathematical properties -- that the entries of minorsMap are the b-by-b minors, that det of hyperdeterminantMatrix is the hyperdeterminant, that pureResES1 yields an Eisenbud-Schreyer pure resolution -- and include error tests for bad input.
Adds three TEST blocks to TorAlgebra.m2 covering exported symbols that previously had no direct test coverage: the predicates isCI, isGorenstein, isGolod; torAlgDataPrint; setAttemptsAtGenericReduction; and the attemptsAtGenericReduction key. The tests use rings of known class (complete intersection, Golod, Gorenstein non-CI), exercise both the Ideal and QuotientRing forms, assert torAlgData hash-table fields directly, and include the documented generic-reduction error path.
Rewrite all 12 TEST blocks of InverseSystems.m2: wrap three silent-pass bare-boolean expressions in assert, drop a compute-and-discard line and a verbatim-duplicated test block, remove dead locals, and add output-type and edge-case checks. All 12 tests verified passing under Macaulay2 1.26.05.
Four TEST blocks had reversed fences (///TEST instead of TEST ///), so M2 parsed them as inert string literals and never ran them. Restore the fences to revive the unprojectionHomomorphism, delta, resBE, and isExactRes tests. The revived unprojectionHomomorphism test asserted equality against a hand-built map whose source-generator order and target ring did not match the actual return value; replace it with order-independent checks on the target ring, source module, and image ideal. Add rank assertions to the kustinMillerComplex(Ideal,Ideal,...) test and remove compute-and-discard lines (fVector, betti) that added no coverage.
Add TEST blocks for all four exports (allgens, bracket, bracketMatrix, ad) -- type, property, concrete-value, boundary, and error tests; previously ad was untested and the rest had only the skew-symmetry and Jacobi-identity tests. Remove the shadowed duplicate bracket(DGAlgebra,ZZ,ZZ) definition and leftover debug prints, and tidy the test section (stray block-comment marker, TEST/// spacing).
Adds three TEST blocks (each debug-ing into the package) covering non-exported helper functions: lengthrow, lengthcolumn, homologicalDegree, tableauInternalDegree and scalarMultiply; the Z/2-graded permutation sign sgn; and columnStandardize together with its sign rule, including the repeated-positive zero case.
Adds a TEST for the non-exported tableau generator standardZ2Tableaux: count identities for a single box (m+n), the cross-check that its count equals the total rank of the corresponding schurComplex, and that every tableau it produces is a fixed point of straightenTableau.
Adds two TEST blocks:
- isWellDefined of schurComplex outputs confirms d^2 = 0, validating
the non-exported differential builder tableauxDiff (the existing
tests check only isHomogeneous)
- straightenTableau / recursiveStraighten projection properties: a
column with a repeated positive entry straightens to zero, and the
partition argument may be passed as a List or a Partition
…fle'
Adds two TEST blocks directly exercising the two most intricate
non-exported straightening helpers:
- permutedTableau: exact-output unit tests for full and partial
column rewrites
- shuffle': the exact shuffling-relation output is pinned for a small
case, and for two cases the relation is verified valid -- straightening
the returned combination recovers straightening the input tableau
The package shipped with 5 TEST blocks that exercised only about a dozen of its 49 exported symbols. Add 19 TEST blocks so every exported function is checked, leaving the original tests untouched. The new tests verify concrete values and structural identities: Young tableau accessors and list conversions; tabloid, standard- and semistandard-tableau generation and their counts; the hook length formula and the sum-of-squares-of-dimensions identity; multinomial coefficients; cycle decompositions, conjugacy classes and class sizes; permutation signs cross-checked against permutation-matrix determinants; group generation; reading words and index tableaux; row descents and column sorting; row and column stabilizers; Garnir relations and the straightening algorithm; SpechtModuleElement arithmetic; Vandermonde and generalized Vandermonde determinants and their Schur-polynomial quotients; Specht, higher Specht, elementary and power-sum symmetric polynomials, including Newton's identities; the matrix representation as a group homomorphism; and the irreducible character table.
Add 8 TEST blocks for exported functions that previously had no direct test assertion: schur, schurModulesMap/weylModulesMap, normalize/augmentFilling, the Weyl tableau helpers (weylNormalize, maxWeylFilling, towardWeylStandard, weylStraighten), dividedPower, divMult/divComult, and characterRep/decomposeRep -- type, property, concrete-value and boundary tests (functoriality, rank formulas, character agreement). The 7 existing tests are unchanged.
Promotes the ciOperatorResolution doc-example to a TEST: it verifies that AL = ciOperatorResolution(A,L) is a homogeneous S-free resolution, exact below its truncated top, of the same module as the minimal resolution G. Exercises higherCIOperators, makeALDifferential, ciOperatorResolution and trueKoszul -- four exports that previously had no direct test.
Adds a TEST verifying that trueKoszul ff is a homogeneous Koszul resolution of S/ideal ff with the same Betti table as koszulComplex but a reordered (exterior-algebra lex) basis.
…iativity Revives the package's checkAssociativity oracle (previously dead code in a top-level string) as a TEST asserting associativity of the exterior multiplication maps for n = 5.
Cover isDirectSummand/isSummand, findProjectors, findIdempotents/findIdem, and frobeniusPullback with type, property, decomposition, and error tests.
incidenceCohomology was the package's thinnest-covered export: two scalar assertions (Tests 30-31), and the intended Test 29 was an empty TEST block that check ran as a no-op. Fill Test 29 with a FindCharacter test: with FindCharacter => true the function returns the cohomology character, whose coefficient sum recovers the dimension returned with FindCharacter => false; this also exercises the (List, PolynomialRing) dispatch over a user-supplied ring. Add five TEST blocks (32-36) for incidenceCohomology: regression against known H^i(X, O_X(a,b)) dimensions, the a<->b twist symmetry, Serre duality H^i(O_X(a,b)) = H^(2n-3-i)(O_X(-n+1-b,-n+1-a)), vanishing in the degenerate twist range and Kempf vanishing for dominant twists, and characteristic-independence of the dominant H^0. Each block carries a header comment. check now runs 37 TEST blocks, up from 32 (one of which -- Test 29 -- was an empty no-op), all passing.
The package has 89 exports vs 9 small TEST blocks. Add a single
focused TEST that exercises five symbols the audit flagged as having
zero direct TEST coverage:
- numParameters on a vanilla PolySystem returns 0.
- numVariables / numFunctions reflect the system's shape.
- realPoints filters complex points: {realPoint, complexPoint}
reduces to {realPoint}.
- numericalAffineSpace and numericalVariety return NumericalVariety
instances on trivial input.
- toAffineChart(0, {a, b, c}) == {b/a, c/a} -- i.e. dehomogenize by
the 0th coordinate (here a == 1.0, so output is {b, c}).
…op joke Caveat
Quality fixes in reesModuleToIdeal:
- Two error strings labeled themselves "internalModuleToIdeal:" but
the exported function is reesModuleToIdeal -- update to match.
- Bare `print "warning"` (no context) at line 321 silently emitted
to stdout. Gate behind `debugLevel > 0` and give an informative
message ("candidate map not injective; trying next generator").
- Stray `print coeffRing` at line 328 (debug breadcrumb not gated
by debugLevel). Gate it too.
Doc fix:
- gradedReesPiece doc Caveat read "This method is peanut-butter-free."
-- a joke placeholder. Drop the empty Caveat block.
The audit flagged two exports that were documented and exercised via
doc examples but had no direct TEST assertion:
- tensorLog: the inverse of tensorExp at the level of truncated
tensors. For x = 1 + Lt_1 + 1/2 Lt_{1,1} + 1/6 Lt_{1,1,1} (i.e. the
truncated exp of Lt_1), the truncated log must give Lt_1 in degree 1
and 0 in degrees 0, 2, 3.
- getCoefficientRing on a Path: linPath({0,1}) gives a Path over QQ;
pin the class === Path and getCoefficientRing === QQ. Also assert
that the installed alias `coefficientRing` agrees with
`getCoefficientRing` on Path inputs.
The [isQuasiIsomorphism, Concentration] doc node needs Concentration in this package's user-facing dict for the doc renderer to serialize it; PackageImports alone places it in the private dict only, which caused "can't convert local symbol or invisible global symbol 'Concentration' to external string" during install-ChainComplexExtras in CI.
bd4c2c8 set OptionalComponentsPresent => true so `check` runs the TEST blocks (none of which call Maple), but that also made M2 default UseCachedExampleOutput to false (packages.m2 L349), so installPackage re-runs the adjointIdeal / geometricGenus doc Examples — both of which do shell out to Maple via MapleInterface — and they fail on CI runners where Maple is not installed. CacheExampleOutput => true had already shipped pre-recorded .out files under packages/AdjointIdeal/examples/ for exactly this case; the fix is to declare UseCachedExampleOutput => true so installPackage picks them up instead of regenerating. TEST blocks remain unaffected and continue to run under `check` because OptionalComponentsPresent stays true. Verified locally: installPackage with IgnoreExampleErrors => false now succeeds (was hitting `error: Maple returned errors`).
kellerlv
changed the title
…s Homebrew gfan The test added in 6f8527a asserts computeMixedVolume {u^3, v^3} == 0, which is mathematically correct (two pure monomials have point Newton polytopes, so no mixed cells exist) but exposes a segfault in the Homebrew-packaged gfan 0.8beta on macOS — the binary called by gfanInterface for _mixedvolume crashes on this degenerate input. The apt-packaged gfan 0.6.2 on Ubuntu handles it correctly, so the bug is in the gfan packaging, not in M2. The cmake-macos-15-brew-clang CI job is the only one that runs check on packages AND uses /opt/homebrew/bin/gfan, so it's the only job where this combination ever surfaced; the other three pass. Drop the offending assertion and leave a comment pointing at the gfan issue so the test can be re-added once the upstream fix lands. The two non-degenerate assertions in the block are unaffected and still exercise the normal mixed-volume code path.
…macOS CI Workshop commit 412afb1 re-enabled the (3,5) case of the Johnson 2003 determinantal multiplier-ideal test on the theory that the historical CI skip was a time-budget issue. Running it on the macOS-cmake CI runner shows the actual constraint is memory, not time: the macOS-15 GitHub runner is more memory-constrained than the Linux runners, and the (3,5) case crashes the M2 process with `*** out of memory trying to allocate 80 bytes, exiting ***` at test #26. On autotools-ubuntu the same test passes in ~12 seconds; on cmake-macos-15-brew-clang it OOMs. Re-comment the three (3,5) assertions and leave an explanatory note in the test block so the next contributor doesn't re-revive it. The (3,3) and (3,4) cases stay enabled (verified locally: both pass in well under a second). Re-enable the (3,5) block only when macOS CI runners gain more RAM or genericDeterminantalSymbolicPower is made more memory- efficient. Per the workshop test-writing guidelines: "Commented-out / silenced TEST blocks are usually disabled for performance. When reviving one: ... if it is too slow [or OOMs], re-comment it (or revive only a lightweight variant)."
Member
|
Beautiful rebasing, thank you! Could you disclose AI use in the top PR message? |
mahrud
reviewed
May 22, 2026
Member
There was a problem hiding this comment.
I presume this is redundant with the TestAudit package PR? Which one should be merged first?
Contributor
Author
There was a problem hiding this comment.
Yes (edited because I misread this originally)
| assert not isDirectSummand(module ideal(x,y), N ++ N) | ||
| -- error: the two modules must be defined over the same ring | ||
| S2 = ZZ/5[a,b,c] | ||
| assert(try (isDirectSummand(S2^1, M); false) else true) |
Contributor
There was a problem hiding this comment.
This test passes if isDirectSummand(S2^1, M) fails for any reason, not just for being defined over the same ring. That's probably fine, but we could also try to test against the error message from isDirectSummand ("expected objects over the same ring").
| assert not isHomogeneous Minhom | ||
| assert(try (findProjectors Minhom; false) else true) | ||
| -- error: an indecomposable module (here R^1) yields no projectors | ||
| assert(try (findProjectors R^1; false) else true) |
Contributor
There was a problem hiding this comment.
Similarly to my previous comment: maybe we should have tests that pass on findProjectors returning null look different than tests that pass when an error is raised.
| -- findIdem is an exported synonym for findIdempotents | ||
| assert(findIdem === findIdempotents) | ||
| -- error: an indecomposable module (here R^1) yields no idempotents | ||
| assert(try (findIdempotents R^1; false) else true) |
| assert(frobeniusPullback(1, vars R) == map(R^1, R^{-3}, {{x^3}})) | ||
| -- the Ring and Ideal inputs reduce to the module case | ||
| assert(frobeniusPullback(1, R) == R^1) | ||
| assert instance(frobeniusPullback(1, ideal x), Module) |
Contributor
There was a problem hiding this comment.
These are great, and perhaps we're happy just having coverage of each function, but it might be worth adding tests that address two of the more subtle settings for frobeniusPullback:
- When R is defined over GF p^e for e > 1,
frobeniusPullbackalso involves a twist of the underlying ring. So maybe a test for the behavior in this setting (e.g., if R = (GF 4)[x]/(x+a), then the first Frobenius pullback of R^1 should be ((GF 4)[x]/(x^2+x+a), etc). - Maybe a test for
frobeniusPullback(ZZ, CoherentSheaf)would be good - e.g., checkfrobeniusPullback(1,frobeniusPushforward(1,OO_X))is trivial whenXis an elliptic curve.
(I feel bad asking you to write tests for things we didn't!)
Contributor
Author
There was a problem hiding this comment.
No worries asking for tests, we are happy to add suggestions from the package authors.
Contributor
Author
There was a problem hiding this comment.
For 1. : I'm a little confused. Where did x^2 + x + a come from? I was expecting the quotient to become (GF 4)[x] / (x + a^2). The current code is giving the quotient by x + a + 1, which I guess makes sense since a^2 = a+1.
For 2: I tried a characteristic 5 example and one subtlety is that we actually get a rank 5 bundle, but this technically makes sense since pushing forward O_X should technically decompose it as a rank 5 bundle, then pulling back each of those summands gives a line bundle, so the output should be rank 5, no? This is also what the code gave when I ran your suggestion.
Contributor
There was a problem hiding this comment.
For 2: sorry, by trivial I just meant direct sum of OO_X, so exactly as you say! (Or maybe there's good tests for pullbacks of the tangent sheaf of P^n, though I don't know what one would hope to be true.)
For 1: You're correct, I intended x + a^2 = x + a + 1 as the denominator.
Contributor
|
Left a few local comments on the DirectSummands tests, but overall I'm happy with the added tests - thanks all! |
…e Frobenius cases Addresses Devlin-Mallory's review comments on PR Macaulay2#4322: - Replace four try/(...; false)else true patterns at lines 491, 513, 515, 537 with capture+match against the specific error string ("expected objects over the same ring", "expected a homogeneous module", "no projectors found", "no idempotent found"). Calibrated against the DirectSummands sources to confirm each function raises an error rather than returning null. - Add a TEST for frobeniusPullback over GF(4): R = (GF 4)[x]/(x+a) pulls back to (GF 4)[x]/(x+a^2), pinning the field-twist a -> a^p that frobeniusTwistMap applies (frobenius.m2:40). - Add a TEST for frobeniusPullback(ZZ, CoherentSheaf) on a smooth elliptic curve over Z/5: F^* F_* OO_X has rank p = 5 over an unchanged ring, exercising the sheaf overload. All 31 DirectSummands TEST blocks pass under \`check\`.
mahrud
reviewed
May 23, 2026
mahrud
reviewed
May 23, 2026
Agreed, not sure why it was written like that. Co-authored-by: Mahrud Sayrafi <sayrafi.m@gmail.com>
mahrud
reviewed
May 23, 2026
Comment on lines
+870
to
+897
| -- Pin the *current* (not ideal) behavior of isomorphism on the example | ||
| -- whose ideal-behavior assertions are silenced at lines 752-784 with a | ||
| -- "TODO: ideally, if a homogeneous isomorphism _can_ be found, we | ||
| -- should find it" breadcrumb. These assertions document the current | ||
| -- behavior so that any future change is surfaced as a regression -- | ||
| -- whether the change is a fix (the silenced asserts become satisfiable | ||
| -- and these regression asserts start failing) or a further drift. | ||
| TEST /// | ||
| debug Isomorphism | ||
| R := QQ[x,y] | ||
| M := coker map(R^{{1}, {0}}, , {{x},{0}}) | ||
| N := coker map(R^{{0}, {1}}, , {{0},{x}}) | ||
| assert isIsomorphic(N, M, Strict => false, Homogeneous => false) | ||
| f := isomorphism(N, M, Strict => false, Homogeneous => false) | ||
| -- Current behavior: the returned isomorphism is NOT homogeneous, | ||
| -- even though M and N are isomorphic in the homogeneous category | ||
| -- (see TODO at Isomorphism.m2:751). | ||
| assert(not isHomogeneous f) | ||
| -- The cache stores the isomorphism under key (N, M, {true, false}) | ||
| -- rather than {true, true} (which is what the silenced assert at | ||
| -- :752 expects). | ||
| assert(N.cache.cache.Isomorphisms#?(N, M, {true, false})) | ||
| assert(not N.cache.cache.Isomorphisms#?(N, M, {true, true})) | ||
| -- A single isomorphism is currently cached. | ||
| assert(1 == #N.cache.cache.Isomorphisms) | ||
| -- And nothing is cached on M (only the larger of the two stores). | ||
| assert(not M.cache.cache.?Isomorphisms) | ||
| /// |
Member
There was a problem hiding this comment.
Huh this is almost identical to the test starting at line 745, except that the variables are defined with := instead of = and it quotes my comments there rather than copying them, and importantly "pins the current behavior" even though it is incorrect.
Isn't this counterproductive, in that it gives the impression that certain code paths are being tested, even though previously they were not being tested because they were incorrect?
Contributor
Author
There was a problem hiding this comment.
Oh yeah -- this was inspired by some discussions about a wished-for "BrokenTest" type or something like that, where one can put known bugs in the broken tests and it'll actually give an error if you do something that fixes the test (then it can be easily converted into a proper TEST). My reasoning here is that sometimes you don't know what a given change will fix and it's useful to have things formally recorded that DO fail so it's easy to tell if you've fixed something, so this is a hack-y version of trying to accomplish this.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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.
PR to run Macaulay2's CI against the accumulated test-suite overhauls from the Atlanta workshop branch.
We are open to suggestions for ways to split this up in a way that is easier to digest for the reviewers.
AI (Claude) was used for multiple tasks such as reformatting, detecting broken/commented out tests, helping trace bugs, and other tedious yet straightforward tasks.