Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complexes pr 3 #3526

Merged
merged 36 commits into from
Nov 18, 2024
Merged

Conversation

mikestillman
Copy link
Member

This pull request includes the next set of minor package changes to get them working with Complexes (the current plan is to make this the default in the April 2025 M2 release, removing the old ChainComplex type from the Core).

Pinged authors: do these changes look ok to you? (They should all be minor: changing ChainComplex to Complex, etc)

After this pull request, there are not too many packages left to transfer to work directly with the new Complexes package.

mikestillman and others added 21 commits October 7, 2024 16:42
…lexes. Also discovered: Complexes doesn't play well with rings with degree length 0.
…s (added new optional argument to minimalBettiNumbers)
…es (added new optional argument to minimalBettiNumbers and bassNumbers)
…. Also removed definition of global symbol 'Range'
@@ -27,7 +27,7 @@ newPackage(
Headline => "routines for working with normal toric varieties and related objects",
Keywords => {"Toric Geometry"},
PackageExports => {"Polyhedra", "Schubert2", "Varieties","Truncations"},
PackageImports => {"FourierMotzkin", "LLLBases"},
PackageImports => {"FourierMotzkin","Normaliz","LLLBases","Complexes"},
Copy link
Member

Choose a reason for hiding this comment

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

NormalToricVarieties doesn't use Normaliz.

@bbarwick
Copy link

@mikestillman (cc: @bstone) The QuillenSuslin.m2 changes look good to me. Thanks!

…One test fails, due to another package not yet changed to work with Complexes.
…ing with Complexes. Potentially subtle change involving mapping cone, which now orders the summands differently
…ionResolutions to get them working with the new Complexes package
…. Needed to add LengthLimit to res in killingCyclesOneStep
@lrtbuse
Copy link
Contributor

lrtbuse commented Oct 22, 2024 via email

@galettof
Copy link
Contributor

Hi Mike,

From what I can tell, the changes are fine for the HighestWeights and BettiCharacters packages.

Thank you!

@mikestillman
Copy link
Member Author

@ggsmith K3Carpets is failing: it is using CompleteIntersectionResolutions. Unfortunately, it doesn't look as trivial to modify as the other packages we have done. It uses also NonminimalComplexes, which we have not yet modified to work with Complexes.

@mahrud
Copy link
Member

mahrud commented Oct 24, 2024

Do you want to revert this PR to commit cfe3b5d, which passed all tests? I think changes to every package up to that point except for KustinMiller and EagonResolutions have been approved by the authors.

The remainder can be added in the next PR perhaps.

@ggsmith
Copy link
Contributor

ggsmith commented Oct 24, 2024

Do you want to revert this PR to commit cfe3b5d, which passed all tests? I think changes to every package up to that point except for KustinMiller and EagonResolutions have been approved by the authors.

The remainder can be added in the next PR perhaps.

Not yet—we're stilling working on it.

@jankoboehm
Copy link
Contributor

Hi Mike, The changes look very good with respect to the KustinMiller package, provided the tests pass of course. Thanks a lot!

…We added a function constantStrands to Complexes, which allowed us to remove the dependency on NonminimalComplexes in K3Carpets
@mikestillman
Copy link
Member Author

@ggsmith I'm not sure I can find the bug in time, before the release gets made. We might need to back off to the last pr-3 pull request...

@mikestillman
Copy link
Member Author

@d-torrance What do you think about this? I'm also concerned that I won't be able to quickly fix the conflict errors after David's PR for PencilsOfQuadrics...

@d-torrance
Copy link
Member

Yeah, holding off on this PR until after the release sounds like a good idea.

@mahrud
Copy link
Member

mahrud commented Oct 31, 2024

In the current version rawMinimalBetti receives a raw computation that hasn't started yet, so here is a fix:

diff --git a/M2/Macaulay2/packages/Complexes/FreeResolutions.m2 b/M2/Macaulay2/packages/Complexes/FreeResolutions.m2
index 27f0bd9aa8..a472fffe0f 100644
--- a/M2/Macaulay2/packages/Complexes/FreeResolutions.m2
+++ b/M2/Macaulay2/packages/Complexes/FreeResolutions.m2
@@ -202,6 +202,7 @@ resolutionObjectInEngine = (opts, M, matM) -> (
         complex maps
         );

+    if not opts.StopBeforeComputation then
     RO.compute(opts.LengthLimit, opts.DegreeLimit);
     RO.complex(opts.LengthLimit)
     )
@@ -691,7 +692,8 @@ minimalBetti Module := BettiTally => opts -> M -> (
        nvars := # generators(R, CoefficientRing => A);
        lengthlimit = nvars + if A === ZZ then 1 else 0;
         );
-    C = freeResolution(M, DegreeLimit => degreelimit, LengthLimit => lengthlimit + 1, Strategy => Nonminimal);
+    C = freeResolution(M, DegreeLimit => degreelimit, LengthLimit => lengthlimit + 1,
+       Strategy => Nonminimal, StopBeforeComputation => true);
     rC := M.cache.ResolutionObject.RawComputation;
     B := unpackEngineBetti rawMinimalBetti(rC,
         if opts.DegreeLimit === infinity then {} else

@mahrud mahrud mentioned this pull request Nov 1, 2024
@mahrud
Copy link
Member

mahrud commented Nov 6, 2024

Mike, is this done? The tests have passed. GitHub is not happy with your last merge commit, which would be resolved if you rebased and force pushed, but we can also just merge this without rebasing.

@mikestillman
Copy link
Member Author

Yes, I'll accept it, but use a merge pull request.

@mikestillman mikestillman merged commit 3c26ddd into Macaulay2:development Nov 18, 2024
5 checks passed
@mikestillman mikestillman deleted the complexes-pr-3 branch November 18, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants