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

Added file: AbstractSimplicialComplexes.m2 #3547

Merged
merged 7 commits into from
Oct 30, 2024
Merged

Added file: AbstractSimplicialComplexes.m2 #3547

merged 7 commits into from
Oct 30, 2024

Conversation

n-m-g
Copy link
Contributor

@n-m-g n-m-g commented Oct 27, 2024

This package gives a way to work with simplicial complexes implemented as graded lists. It complements the existing M2 package SimplicialComplexes.m2.

@n-m-g
Added file: AbstractSimplicialComplexes.m2
bf22b29 
This package gives a way to work with simplicial complexes implemented as graded lists.  It complements the existing M2 package SimplicialComplexes.m2.
@d-torrance d-torrance changed the base branch from master to development October 28, 2024 00:31
@d-torrance
Copy link
Member

I went ahead and fixed it for you, but for future reference, pull requests should target the development branch.

@d-torrance d-torrance mentioned this pull request Oct 28, 2024
Closed
@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 28, 2024 via email

@d-torrance
Copy link
Member

You're welcome!

A couple other issues:

  • AbstractSimplicialComplexes should be added to the file M2/Macaulay2/packages/=distributed-packages
  • There are some conflicts in SpectralSequences between your branch and the development branch

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 28, 2024 via email

@d-torrance
Copy link
Member

I'm not sure if I'm familiar enough with the code to be of much help... Here's the big conflict:

<<<<<<< HEAD
    C = chainComplex complex L#0; -- By default the ambient simplicial complex is the first element of the list
    maps = apply(#L-1, p -> map(C, chainComplex complex L#(p+1), 
        i -> sub(contract(transpose matrix{faces(i,L#0)}, matrix{faces(i,L#(p+1))}), kk))))
    else (C = truncate(chainComplex complex L#0,1); -- By default the ambient simplicial complex is the first element of the list
    maps = apply(#L-1, p -> map(C, truncate(chainComplex complex L#(p+1),1), 
        i -> sub(contract(transpose matrix{faces(i,L#0)}, matrix{faces(i,L#(p+1))}), kk))))   
=======
    C = complex chainComplex L#0; -- By default the ambient simplicial complex is the first element of the list
    maps = apply(#L-1, p -> map(C, complex chainComplex L#(p+1), 
	    i -> sub(contract(transpose matrix{faces(i,L#0)}, matrix{faces(i,L#(p+1))}), kk))))
    else (C = naiveTruncation(complex chainComplex L#0,1,infinity); -- By default the ambient simplicial complex is the first element of the list
--- the ``patch method" -- truncate a chain complex at a given homological degree 
---- truncate(ChainComplex,ZZ) is now replaced by naiveTruncation(Complex,ZZ,ZZ)
   maps = apply(#L-1, p -> map(C, naiveTruncation(complex chainComplex L#(p+1),1,infinity), 
        i -> sub(contract(transpose matrix{faces(i,L#0)}, matrix{faces(i,L#(p+1))}), kk))));   
>>>>>>> 9300cc5cb (Update SpectralSequences.m2)

This looks like it's running into the complexes revamp that @mikestillman has been working on. The top part is the part of the code that was changed in #3479 , and the bottom part is your new updates.

@d-torrance
Copy link
Member

There were also a bunch of examples changed in #3479, which is causing a conflict with the documentation, as well.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 28, 2024

Perhaps it's worthwhile mentioning, that both the new package/file AbstractSimplicialComplexes.m2 and the SpectralSequences.m2 "forward compatible" update install and run correctly on Macaulay2, version 1.24.05.

@mahrud
Copy link
Member

mahrud commented Oct 28, 2024 •
edited
Loading

Perhaps it's worthwhile mentioning, that both the new package/file AbstractSimplicialComplexes.m2 and the SpectralSequences.m2 "forward compatible" update install and run correctly on Macaulay2, version 1.24.05.

There are references to SpectralSequences2.m2 in the new version, but this file doesn't exist, so I suspect your tests work correct locally because you have some other local files that are not included here.

Also, I noticed all the tests start like:

TEST ///
restart;
needsPackage "SpectralSequences";
...

The restart there causes the test to terminate on the first line, so none of the tests have actually been running. This probably should have flagged some sort of error. (Side note: you also don't need to load the package in its own examples or tests, so the needsPackage lines are also not necessary, but not harmful either)

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024

Yes. These tests all run OK but test, the restart should be removed!

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024

Did you want me to update the SpectralSequences.m2 to remove the "restart" from the test and issue a new pull request, or is this something that you can do on your end? Do let me know.

@mahrud
Copy link
Member

mahrud commented Oct 29, 2024

Yes, you should remove the restarts, resolve the conflicts with the development branch, fix the issue with missing "SpectralSequence2.m2" files. Doug also requested a few changes above for AbstractSimplicialComplexes that should be implemented.

You don't need to make a new pull request, just implement the changes, commit the changes, and push them to your fork. They will be added here.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 29, 2024

The new pull request that you made looks good. Here, I think you should remove SpectralSequences and leave this PR to be just about AbstractSimplicialComplexes.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024

Hi,

AbstrastSimplicialComplexes is good to go now (unless there are other comments); I updated SpectralSequences again so that I have the same file on my fork (but this is now on the development branch now so its fine there as well).

@d-torrance
Copy link
Member

This branch still has a conflict with SpectralSequences.m2. Since #3552 now exists with the updates to that package, I think it would make sense to remove the SpectralSequences changes from this branch so that it only deals with the new AbstractSimplicialComplexes package.

There are a number of ways to do that, but one way would be:

git checkout master
git checkout development -- M2/Macaulay2/packages/SpectralSequences.m2
git add M2/Macaulay2/packages/SpectralSequences.m2
git commit -m "Revert SpectralSequences changes"
git push origin master

@d-torrance
Copy link
Member

Also, a line containing the package name should be added to M2/Macaulay2/packages/=distributed-packages.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@d-torrance
Copy link
Member

Ah, ok. You should be able to edit M2/Macaulay2/packages/=distributed-packages using the GitHub web interface just like you edited the .m2 files, so that's not a problem.

I guess the easiest way to restore SpectralSequences.m2 using the web interface would just be to copy/paste it from its current state on the development branch:

https://raw.githubusercontent.com/Macaulay2/M2/refs/heads/development/M2/Macaulay2/packages/SpectralSequences.m2

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@d-torrance
Copy link
Member

SpectralSequences.m2 is still showing some changes (although there are no more conflicts). I'll go ahead and remove those commits for you using the command line -- the git history will be cleaner that way without a bunch of SpectralSequences commits that are eventually reverted.

M2/Macaulay2/packages/=distributed-packages still needs to be updated to add the line AbstractSimplicialComplexes.

@d-torrance
Copy link
Member

On the development branch, the Polyhedra package is now one of the preloaded packages, which is causing the conflict. You can probably reproduce the error on your system by running:

needsPackage "Polyhedra"
needsPackage "AbstractSimplicialComplexes"

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@n-m-g
Update AbstractSimplicialComplexes.m2
9caa816 
renamed "facets" to "abstractSimplicialComplexesFacets" to avoid key space naming conflicts (and to avoiding overloading facets for now).
@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@n-m-g
Update AbstractSimplicialComplexes.m2
9be2c9d 
fixed the "codespell" issue again.
@d-torrance
Copy link
Member

Yes, that should work

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 29, 2024 via email

@d-torrance
Copy link
Member

The builds all worked! Thanks!

@d-torrance d-torrance merged commit 490000a into Macaulay2:development Oct 30, 2024
5 checks passed
@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024 •
edited
Loading

I noticed a comment in the code:

we could overload "==" here but it seems better not to overload too many things that are external to the package

Why do you say that? I generally think having specialized method names makes it harder for people to learn how to use the package (e.g. if one package uses areEqual, another uses isEqual, another equality, another just ==, etc.)

If you don't mind, can I change abstractSimplicialComplexFacets back to facets, dimAbstractSimplicialComplex to dim, and areEqual to just ==?

This is called "idiomatic programming" and is generally considered a good practice.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

dim and == are quite stable and have been for as long as I know. facets is defined in Polyhedra and hasn't changed in years either. The only change is that Polyhedra is now preloaded, so the only change needed in your package was to delete facets = method() because Polyhedra already does that.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

Sure, if you prefer things the way they are that's fine, though I think there's a misunderstanding because using method name facets doesn't mean there's a dependence on Polyhedra, it's just a name.

i4 : about facets

o4 = {0 => FourierMotzkin :: Finding the facets of the cyclic polytope}
     {1 => FourierMotzkin :: Finding the facets of the permutahedron  }
     {2 => Polyhedra :: facets                                        }
     {3 => Polyhedra :: facets(Cone)                                  }
     {4 => Polyhedra :: facets(Polyhedron)                            }
     {5 => SimplicialComplexes :: facets(SimplicialComplex)           }
     {6 => SRdeformations :: facets                                   }
     {7 => SRdeformations :: facets(Complex)                          }
     {8 => SRdeformations :: facets(List)                             }

Similarly, tons of packages use dim:

i3 : about dim

o3 = {0 => CellularResolutions :: dim(Cell)                        }
     {1 => CellularResolutions :: dim(CellComplex)                 }
     {2 => Chordal :: dim(ChordalNet)                              }
     {3 => Chordal :: dim(ChordalNetChain)                         }
     {4 => CodingTheory :: dim(LinearCode)                         }
     {5 => CoincidentRootLoci :: dim(CoincidentRootLocus)          }
     {6 => GradedLieAlgebras :: dim(List,ExtAlgebra)               }
     {7 => GradedLieAlgebras :: dim(List,LieAlgebra)               }
     {8 => GradedLieAlgebras :: dim(List,VectorSpace)              }
     {9 => GradedLieAlgebras :: dim(ZZ,ExtAlgebra)                 }
     {10 => GradedLieAlgebras :: dim(ZZ,LieAlgebra)                }
     {11 => GradedLieAlgebras :: dim(ZZ,VectorSpace)               }
     {12 => GradedLieAlgebras :: dim(ZZ,ZZ,ExtAlgebra)             }
     {13 => GradedLieAlgebras :: dim(ZZ,ZZ,LieAlgebra)             }
     {14 => GradedLieAlgebras :: dim(ZZ,ZZ,VectorSpace)            }
     {15 => InvariantRing :: dim(GroupAction)                      }
     {16 => KustinMiller :: dim(Face)                              }
     {17 => LieTypes :: dim(LieAlgebra)                            }
     {18 => LieTypes :: dim(LieAlgebraModule)                      }
     {19 => LocalRings :: dim(LocalRing)                           }
     {20 => Macaulay2Doc :: dim                                    }
     {21 => Macaulay2Doc :: dim(FractionField)                     }
     {22 => Macaulay2Doc :: dim(GaloisField)                       }
     {23 => Macaulay2Doc :: dim(Ideal)                             }
     {24 => Macaulay2Doc :: dim(InexactField)                      }
     {25 => Macaulay2Doc :: dim(Module)                            }
     {26 => Macaulay2Doc :: dim(MonomialIdeal)                     }
     {27 => Macaulay2Doc :: dim(PolynomialRing)                    }
     {28 => Macaulay2Doc :: dim(ProjectiveHilbertPolynomial)       }
     {29 => Macaulay2Doc :: dim(QuotientRing)                      }
     {30 => Macaulay2Doc :: dim(Ring)                              }
     {31 => MultiprojectiveVarieties :: dim(MultiprojectiveVariety)}
     {32 => NAGtypes :: dim(Ambient)                               }
     {33 => NAGtypes :: dim(DualSpace)                             }
     {34 => NAGtypes :: dim(NumericalVariety)                      }
     {35 => NAGtypes :: dim(PolySpace)                             }
     {36 => NAGtypes :: dim(SlicingVariety)                        }
     {37 => NAGtypes :: dim(WitnessSet)                            }
     {38 => NAGtypes :: dim(WSet)                                  }
     {39 => NormalToricVarieties :: dim(NormalToricVariety)        }
     {40 => OldPolyhedra :: dim(Cone)                              }
     {41 => OldPolyhedra :: dim(Fan)                               }
     {42 => OldPolyhedra :: dim(PolyhedralComplex)                 }
     {43 => OldPolyhedra :: dim(Polyhedron)                        }
     {44 => Polyhedra :: dim(PolyhedralObject)                     }
     {45 => Schubert2 :: dim(AbstractVariety)                      }
     {46 => Schubert2 :: dim(AbstractVarietyMap)                   }
     {47 => SchurRings :: dim(List,SchurRingElement)               }
     {48 => SchurRings :: dim(SchurRingElement)                    }
     {49 => SchurRings :: dim(Thing,SchurRingElement)              }
     {50 => SimplicialComplexes :: dim(SimplicialComplex)          }
     {51 => SparseResultants :: dim(MultidimensionalMatrix)        }
     {52 => SRdeformations :: dim(Complex)                         }
     {53 => SRdeformations :: dim(Face)                            }
     {54 => SRdeformations :: dim(Face,Complex)                    }
     {55 => SRdeformations :: dim(Face,PolynomialRing)             }
     {56 => SRdeformations :: dim(FirstOrderDeformation)           }
     {57 => TriangularSets :: dim(TriaSystem)                      }
     {58 => Tropical :: dim(TropicalCycle)                         }
     {59 => Varieties :: dim(AffineVariety)                        }
     {60 => Varieties :: dim(ProjectiveVariety)                    }

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

For future reference, I also want to point out the Package Writing Style Guide which states:

Names representing methods [...] should not include the name of the type of object expected as argument, since the idea of such methods is that they are mathematical abstractions that act on a variety of types of mathematical object.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

dimAbstractSimplicialComplex includes the name of the type of object expected as input, which is AbstractSimplicialComplex, so this is explicitly against the style guide. Neither dim, facets, nor == accept any options either, so that can't be the reason why the previous build didn't work.

Again, I'm not suggesting any integration with Polyhedra whatsoever, and you're welcome to have your package as is, but I am not aware of issues with overloading these methods or any confusion that can be caused as a result.

@d-torrance
Copy link
Member

Perhaps facets should be moved to shared.m2 in Core.

FWIW, the (non-abstract) SimplicialComplexes package has Polyhedra in its PackageExports and defines a facets(SimplicalComplex) method.

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

We moved cone and rays to shared.m2 for the same reason, as well as isEmpty and isVeryAmple recently.

I believe SimplicialComplexes exports Polyhedra because they both use terms like skeleton, facets, vertices, etc., but it doesn't actually depend on Polyhedra or integrate with it in any way (at least as far as I can tell).

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 30, 2024 via email

@mahrud
Copy link
Member

mahrud commented Oct 30, 2024

Yes, some changes definitely need to be made before the next release, for instance the keywords added should be chosen from existing keywords which are the headlines on this page:

    Keywords => {"Simplicial complexes", "Simplicial chain complexes", "Random simplicial complexes"}

We can discuss the other issues on Zulip if you prefer, but the reasons you mentioned so far (e.g. optional argument conflicts or potential dependency on unstable package) don't apply to dimAbstractSimplicialComplex.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 31, 2024

Sure. I am happy to make changes as required to have everything approved. Yes, "dim" is another one of those overloaded methods that gets used in different senses at times. I'm not sure what you mean by choosing keywords from existing key words. In preparing this package I just followed what we did in the spectral sequences package. We didn't choose any keywords etc. Just let me know what changes you would like me to make and I can try to do this prior to the next release.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 31, 2024

Do you mean what "subject area keywords" it should fall into? For that, I would use "homological algebra" as primary and maybe "combinatorics" and/or "combinatorial commutative algebra" as secondary "subject area keywords".

@d-torrance
Copy link
Member

The Keywords option to newPackage should ideally be chosen from the keywords for existing packages, which can be seen as the headings at the packages provided with Macaulay2 documentation page.

I went ahead at took the liberty of changing the keyword to "Combinatorial Commutative Algebra" (to match the non-abstract SimplicialComplexes package) in #3535.

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 31, 2024 via email

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 31, 2024

If it's not too late, I just did the small edits to overload == and dim as requested as well. I've just committed those changes.

@d-torrance
Copy link
Member

Sure! Feel free to open a new pull request

@n-m-g
Copy link
Contributor Author

n-m-g commented Oct 31, 2024 via email

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

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

Successfully merging this pull request may close these issues.
3 participants
@n-m-g @d-torrance @mahrud