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.

Sign up for GitHub

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

Jump to bottom

Complexes for version 1-24-11, part 1. #3440

Merged
merged 11 commits into from
Sep 11, 2024
Merged

Complexes for version 1-24-11, part 1. #3440

merged 11 commits into from
Sep 11, 2024

Conversation

mikestillman
Copy link
Member

This is the first PR for converting the ChainComplex and ChainComplexMap types from the Core to the types Complex, ComplexMap in Complexes package. Here are the steps we envision to do the switch:

  • (This pull result) Make changes to Complexes and minor changes elsewhere so that it will be easy to set Complexes to be a preloaded package. Also includes some bug fixes and minor improvements.
  • A series of PR's that change each package to run with Complexes, rather than ChainComplexes.
  • Finally, we remove ChainComplex and ChainComplexMap from the Core, including m2/res.m2, m2/chaincomplexes.m2, and move/remove documentation and tests from the Core.
  • Make Complexes a preloaded package.
  • Probably (needs discussion), change the names Complex, ComplexMap in Complexes, back to ChainComplex, ChainComplexMap. Rename the Complex package to ChainComplexes.

@mikestillman
bug fixes for Complexes, add LengthLimit to connectingExtMap, connect…
0f14de5 
…ingTorMap, horsehoeResolution. Fixed bug in mult of complex maps with numbers, bug in homomorphism
@mikestillman
add in epicResolutionMap, beginning of some doc for standard isomorph…
fd7c33b 
…isms
@mikestillman
export epicResolutionMap, remove dead code
5346023
@mikestillman
make isNormal and normalCone method names in m2/shared.m2. This allow…
2da3463 
…s us to remove the dependency of Polyhedra on IntegralClosure and ReesAlgebra (which in turn was making the incorporatino of the Complexes package quite difficult).
@mikestillman
fix normalCone use in Polyhedra and OldPolyhedra, so that examples wi…
30d79ad 
…ll run correctly.
@mikestillman
remove interdependency between FourTiTwo and Polyhedra: hilbertBasis
68d262b
@mikestillman
change complex to a method with Options => true. Add some Complex f…
87975bc 
…unctions: status, rank, transpose, nullhomotopy (synonym of nullHomotopy). Allow complex to take a single matrix. Do not compute remainder in one version of nullHomotopy(serious performance degradation). Moved Ext(Module, Module) functionality from the Core to Complexes. Added minimalBetti fixes.
@mikestillman
Fix 2 bugs in Complexes: specialized routine for null homotopy with f…
1d99126 
…ree source was incorrect. freeResolution(Strategy => Syzygies) incorrectly handled resolutions of length 0. Tests and doc were updated for these.
mahrud
mahrud requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@mahrud mahrud left a comment

Choose a reason for hiding this comment

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

I only reviewed the non-Complexes parts of this PR.

M2/Macaulay2/m2/shared.m2 Show resolved Hide resolved
M2/Macaulay2/packages/ReesAlgebra.m2 Show resolved Hide resolved
M2/Macaulay2/packages/ReesAlgebra.m2 Outdated Show resolved Hide resolved
M2/Macaulay2/packages/ReesAlgebra.m2 Show resolved Hide resolved
M2/Macaulay2/packages/ReesAlgebra.m2 Show resolved Hide resolved
@Devlin-Mallory
Copy link
Contributor

I noticed what I think is a small bug - I was going to make a separate pull request but maybe it makes more sense to change it as part of this PR:

In the "extend" method in ChainComplexMap.m2, lines 795 -- 799 are intended to check if the output of the preceding lines is really a map of chain complexes. However, since line 795 is if false and opts.Verify, the user has no way to actually trigger this, no matter what the value of Verify is. I would suggest simply removing "false and", but is a comment saying "TODO: the following line: "false and" should be removed when we switch Verify to have default value false", so perhaps I am missing some context.

antonleykin
antonleykin reviewed Aug 29, 2024
View reviewed changes
Copy link
Contributor

@antonleykin antonleykin left a comment

Choose a reason for hiding this comment

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

This is a DEMO!

@Macaulay2 Macaulay2 deleted a comment from antonleykin Aug 29, 2024
@mikestillman
fixed nullHomotopy by adding a new optional argument FreeToExact, and…
325328b 
… changed calls in AInfinity to use this simpler algorithm. Also added in doc for associatedGradedRing
@mikestillman
Copy link
Member Author

@d-torrance Any reason why this one Core test is failing? I don't see any artifacts with the failing tests. It runs fine on my mac too... It doesn't seem to me like it uses too much memory either?

@d-torrance
Copy link
Member

This is the failing test:

i1 : tests(73, "Core")

o1 = TestInput[/usr/share/doc/Macaulay2/Core/tests/ext-total.m2:1:1]

I see that this PR moves some of the Ext code to the Complexes package. Could that be affecting the computation somehow? It also runs just fine on my system.

@mikestillman
changes to documentation, including use of ofClass and Hom to \operat…
6884cfb 
…orname{Hom}, imported a test from Core related to total Ext
@mikestillman
Copy link
Member Author

This is the failing test:

i1 : tests(73, "Core")

o1 = TestInput[/usr/share/doc/Macaulay2/Core/tests/ext-total.m2:1:1]

I see that this PR moves some of the Ext code to the Complexes package. Could that be affecting the computation somehow? It also runs just fine on my system.

The problem seemed to be the changes we made in nullHomotopy. The general case was slower (and probably took significantly more memory), so we added in the keyword FreeToExact in two calls to nullHomotopy in Ext(Module,Module). However, with that change, the code would not run on the version before Complexes is loaded. So we moved the test to where it should be anyway (and occurs after Complexes is loaded).

@mikestillman
Copy link
Member Author

After this is merged, we will make a second PR that fixes about 8-10 of the packages, the ones that require mostly trivial changes. After that, another group... continuing as in the first comment of this PR.

@mikestillman
Change the use of e.g. {Module} to {Complex,Module}. Currently, this …
532650e 
…then displays that the argument is a complex, a complex, a module (doubled complex). It appears that is a bug that will get fixed before the next version, so we change this to be correct once that bug fix is in.
@mikestillman
Copy link
Member Author

I made the small doc changes. The github tests are rerunning.

@mahrud
Copy link
Member

mahrud commented Sep 10, 2024

I think this is ready to merge. Should I do it or do you want to?

@mikestillman
Copy link
Member Author

@d-torrance What else should we do before this PR is merged?

@d-torrance
Copy link
Member

Looks good to me!

@d-torrance d-torrance merged commit 646d715 into Macaulay2:development Sep 11, 2024
5 checks passed
@mikestillman
Copy link
Member Author

Should I just do the merge? Should I do "rebase and merge"? Will that screw up future PR's based on the same branch?

@mikestillman
Copy link
Member Author

Oops, all merged! Thanks!

@d-torrance
Copy link
Member

No problem! Using the same branch for future PR's should be fine.

@mikestillman mikestillman deleted the complexes-1-24-11 branch September 13, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-torrance d-torrance d-torrance left review comments

@mahrud mahrud mahrud approved these changes

@antonleykin antonleykin Awaiting requested review from antonleykin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mikestillman @Devlin-Mallory @d-torrance @mahrud @antonleykin