Macaulay2 / M2

The primary source code repository for Macaulay2, a system for computing in commutative algebra, algebraic geometry and related fields.
https://macaulay2.com
347 stars 231 forks source link

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

Closed mikestillman closed 2 months ago

mikestillman commented 2 months ago

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:

Devlin-Mallory commented 2 months ago

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.

mikestillman commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

mahrud commented 2 months ago

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

mikestillman commented 2 months ago

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

d-torrance commented 2 months ago

Looks good to me!

mikestillman commented 2 months ago

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 commented 2 months ago

Oops, all merged! Thanks!

d-torrance commented 2 months ago

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

d-torrance commented 2 weeks ago

@mikestillman, @ggsmith: Do you have any suggestions for how the complexes changes should be described in the changelog for the new release?