M2 icon indicating copy to clipboard operation
M2 copied to clipboard

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

Open mikestillman opened this issue 1 year ago • 1 comments

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 avatar Aug 26 '24 20:08 mikestillman

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.

Devlin-Mallory avatar Aug 28 '24 20:08 Devlin-Mallory

@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?

mikestillman avatar Sep 06 '24 19:09 mikestillman

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.

d-torrance avatar Sep 06 '24 20:09 d-torrance

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 avatar Sep 10 '24 01:09 mikestillman

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 avatar Sep 10 '24 01:09 mikestillman

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

mikestillman avatar Sep 10 '24 02:09 mikestillman

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

mahrud avatar Sep 10 '24 09:09 mahrud

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

mikestillman avatar Sep 11 '24 01:09 mikestillman

Looks good to me!

d-torrance avatar Sep 11 '24 01:09 d-torrance

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 avatar Sep 11 '24 01:09 mikestillman

Oops, all merged! Thanks!

mikestillman avatar Sep 11 '24 01:09 mikestillman

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

d-torrance avatar Sep 11 '24 01:09 d-torrance

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

d-torrance avatar Oct 31 '24 13:10 d-torrance