-
Notifications
You must be signed in to change notification settings - Fork 261
Complexes, OldChainComplexes, and HomologicalAlgebraPackage #3774
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
Conversation
7d2e9a1 to
73732f0
Compare
a4e0f3c to
58a2770
Compare
|
@mikestillman I think this is ready. |
|
I compared the entire html documentation before and after this change, and feel pretty confident that we didn't miss anything major. Do we think this can make it into this release? It should hopefully not change anything by default, but provide the option of testing the complete switch to Complexes by setting a variable in init.m2. Since the tests passed already, I'll wait to see if you have anything else that needs changed before I fix the conflicts. |
Taking a look! |
|
@mahrud Where is the list of preloaded packages? I can't seem to find it... edit: found it, it is in edit: aargghhh... on wrong branch... Found it all... (in |
|
@mahrud Maybe we should have |
If it is not a valid package name, they still get an error that the package doesn't exist, right? |
This is exactly why I moved it to Core.m2 XD I always had to grep for "preloaded packages". |
|
@mahrud So far, most everything is looking good. There are a few small issues so far. In trying to get Complexes to be preloaded, and not OldChainComplexes, I hit the following issues (I might edit this list later).
I am looking into the last one. Should I commit the first two (edit: only the second) small changes, and push to your branch? I didn't try to fix the conflicts with development, I was just testing the branch on its own. |
|
@mahrud I have a few things I want to check, but this is basically good. Please continue with resolving the conflicts. Yes, I would really like this to be in the upcoming release if possible. (edit: I did not yet add any commits to the branch) |
Exactly, I had a commit where I changed
Sure, alternatively we could replace it with a grobner basis or something.
The simple fix is mahrud@65a7209. I have a branch here where I made Complexes the default homological algebra package and fixed all three of these, but I think for this release it's fine. I will include the fix for the third one in this PR though. |
SchurComplexes RandomSpaceCurves RandomCurvesOverVerySmallFields
mainly to make a test in Saturation work with both packages.
|
@mikestillman @d-torrance is this good to merge? |
|
Greg and I have reviewed most of the changes, and this looks good. As long as the googletest submodule issue has been fixed, this looks good to be merged! |
The major changes here include:
Options => trueat some point.resolution(Complex)tofreeResolution(Complex)to avoid having Complexes depend on OldChainComplexes. I believe Mike and Greg were okay with this.Things that may need to be changed later:
formation(Complex)for a test to pass, but it needs to bed documented, andformation(ComplexMap)probably also needs to be addedGradedModuleas a synonym ofComplexfor a few packages which only needed this symbol as the output ofHH, for instance. A better solution might be to create a newGradedModuletype in Complexes and overwrite the other symbol, then the documentation would reflect this as well.LLLas a strategy for free resolutions over ZZ, but it seems like this changed the result, since OverZZ already existed, and changed the documentation nodeStrategy for free resolutions over the integers. I will comment this out for now, but should this also be changed in OldChainComplexes? What was the point of the LLL strategy to begin with? c.f. Free resolutions over ZZ using LLL #3785.[betti, Minimize]doesn't do anything, nor is it documented in Complexes. Should it be? Same with[extend, Verify]