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.

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

sage.{topology,homology}: Modularization fixes #35581

Merged
merged 24 commits into from
May 28, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 28, 2023

📚 Description

sage.topology is closely linked to functionality from sage.graphs. Doctests that either use methods or examples that require sage.graphs are marked # optional - sage.graphs; likewise for sage.groups (for multiplicative groups) and (less often) sage.combinat.
Doctests that use finite fields are marked as # optional - sage.rings.finite_rings because implementation classes of GF(...) depend on various libraries.
Doctests that compute homology are marked as # optional - sage.modules because sage.homology needs vector spaces. This functionality is not part of the distribution sagemath-categories but a higher up distribution (as of #35095, the distribution sagemath-polyhedra contains sage.modules, sage.matrix, sage.homology).
Doctests involving free resolutions are marked # optional - sage.libs.singular.

Some examples are read from data files; the corresponding tests are now marked # optional - pyparsing.

A sporadic use of a function from numpy has been replaced by an equivalent itertools function.

The correct placement of the # optional tags can be (and has been) tested using #35095.

While going through the doctests, I also reformatted some so that they can be read in our HTML documentation style (furo) without having to scroll horizontally.

Part of:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe force-pushed the sage_topology_homology_modularization branch from f806616 to 7b24c69 Compare April 30, 2023 18:30
@@ -552,12 +552,12 @@ def _remove_pivot_rows(self, s, simplices):
sage: l = [([0], 0), ([1], 0), ([2], 1), ([3], 1), ([0, 1], 1), ([1, 2], 1),
....: ([0, 3], 2), ([2, 3], 2), ([0, 2], 3), ([0, 1, 2], 4)]
sage: X = FilteredSimplicialComplex(l)
sage: X._persistent_homology()
sage: X._persistent_homology() # optional - sage.rings.finite_rings
[[(0, 1), (1, 2), (0, +Infinity)], [(3, 4), (2, +Infinity)], []]
Copy link
Collaborator

Choose a reason for hiding this comment

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

No sage.modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, I'll add it.

sage: C2.simplified()
sage: RP2 = simplicial_complexes.RealProjectiveSpace(2) # optional - sage.libs.pari
sage: C2 = RP2.fundamental_group(simplify=False); C2 # optional - sage.graphs sage.groups sage.libs.pari
Finitely presented group < e0, e1, e2, e3, e4, e5, e6, e7, e8, e9 | e0, e3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does L4090 need sage.libs.pari? Or is it L4091?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a mistake. I have removed sage.libs.pari from all lines in this block

@@ -4252,27 +4260,28 @@ def automorphism_group(self):
EXAMPLES::

sage: S = simplicial_complexes.Simplex(3)
sage: S.automorphism_group().is_isomorphic(SymmetricGroup(4))
sage: S.automorphism_group().is_isomorphic(SymmetricGroup(4)) # optional - sage.graphs
True

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this sage.graphs or sage.groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both; I'll fix it

{0: 0, 1: 0, 2: Z x Z, 3: Z}
sage: len(P.nondegenerate_simplices())
Copy link
Collaborator

Choose a reason for hiding this comment

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

No sage.modules or sage.groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of sage.modules are missing in this file. Adding now

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant L1226.

sage: susp_eta.mapping_cone().homology() == eta.mapping_cone().suspension().homology()
sage: mc_susp_eta = eta.suspension().mapping_cone() # optional - sage.graphs
sage: susp_mc_eta = eta.mapping_cone().suspension() # optional - sage.graphs
sage: mc_susp_eta.homology() == susp_mc_eta.homology() # optional - sage.graphs
True
Copy link
Collaborator

Choose a reason for hiding this comment

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

No sage.modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, added.

@kwankyu
Copy link
Collaborator

kwankyu commented May 24, 2023

Though I looked through all changes, definitely I could not check everything. However, as I understand it, these optional tags are not harmful even they are misplaced, but could just be less useful to test distribution packages. Misplaced tags may be corrected when we start to test all distribution packages before Sage release. Am I right?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 24, 2023

as I understand it, these optional tags are not harmful even they are misplaced, but could just be less useful to test distribution packages. Misplaced tags may be corrected when we start to test all distribution packages before Sage release. Am I right?

Right. For testing the monolithic Sage library, there is no change at all. (The only thing that could go wrong is to misspell a # optional tag, which would silently disable the test.)
I am testing these annotations with sagemath-categories and sagemath-polyhedra from #35095. The goal there is to get most tests of these distributions to pass; the annotations do not have to be perfect for that.

@github-actions
Copy link

Documentation preview for this PR (built with commit c4c593a) is ready! 🎉

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

Thanks.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 25, 2023

Thanks for the review!

@vbraun vbraun merged commit cb2f3c0 into sagemath:develop May 28, 2023
@mkoeppe mkoeppe added this to the sage-10.1 milestone May 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants