Extended `Tangle` to allow for a different number of top/bottom strands

3-manifolds / Spherogram

Spherogram is a Python module for dealing with the kind of planar diagrams that arise in 3-dimensional topology, such as link and Heegaard diagrams. It a component of the larger SnapPy project.

https://snappy.math.uic.edu/spherogram.html

19 stars 8 forks source link

Extended `Tangle` to allow for a different number of top/bottom strands #28

Closed kmill closed 1 year ago

kmill commented 2 years ago

This change makes it so that you can interact with the full tangle category rather than just its endomorphisms.

Also:

reversed the composition order for __mul__ to match the convention in doc.pdf
added CapTangle, CupTangle, and BraidTangle
added Tangle.link for (0, 0)-shaped tangles
added Tangle.reshape to rotate and change boundary
added Tangle.describe to give a textual description of a tangle
changed basic tangle definitions to be factory functions rather than classes
added more documentation

kmill commented 2 years ago

This passes sage -python -m spherogram.test but I think it should definitely have more tests. I thought I'd create this PR to check in with what I've been up to here.

kmill commented 2 years ago

@culler For __add__ it was just the documentation that was backwards, and the implementation seems like it was otherwise correct. I could generalize the CupTangle and CapTangle to give those -- maybe it would be worth having similar crossing generators, too, to make creating braids easier?

On second thought, there are a couple generalizations of CupTangle and CapTangle, so maybe it would be better to keep them being simple primitives. A generalization I was thinking of is taking three bundles of parallel strands with a strands, b strands, and c strands, then having the (a+c,a+2b+c) and (a+2b+c,a+c) tangles.

The CupTangle and CapTangle can also now be gotten from IdentityBraid(1).reshape((0,2)) and IdentityBraid(1).reshape((2, 0)), so they might even just be superfluous (though convenient).

NathanDunfield commented 2 years ago

The failed checks are because snappy is not installed as part of the CI script. This is intensional so that the spherogram tests are completely self-contained. It is possible to write snappy-dependent tests that are run instead by snappy.test. See spherogram_src/links/test/test_montesinos.py for an example.

NathanDunfield commented 2 years ago

Just checking in to see what the plan is for this PR. Is it just the tests that need fixing or is more work planned?

kmill commented 1 year ago

@NathanDunfield I'm just getting back to this -- the plan is to fix the tests and to make sure this change doesn't break SnapPy.

kmill commented 1 year ago

@NathanDunfield I think it's ready for review now. I put the SnapPy tests behind the +SNAPPY flag, which I've tested with and without the snappy module being available.

Concerningly, for my version of SnapPy there is a single error when running snappy.test:

File "/Users/kmill/.sage/local/lib/python3.10/site-packages/spherogram/links/tangles.py", line 219, in spherogram.links.tangles.Tangle.numerator_closure
Failed example:
    BraidTangle([1,1,1]).braid_closure().exterior().identify() # doctest: +SNAPPY
Expected:
    [3_1(0,0), K3a1(0,0)]
Got:
    []

The same test succeeds from within spherogram.test (and I've checked that it's actually being run!)

NathanDunfield commented 1 year ago

Concerningly, for my version of SnapPy there is a single error when running snappy.test:

SnapPy's ability to identify non-hyperbolic manifolds is hit or miss and involves considerable randomization and hence stochastic outcomes. Changing the example to a hyperbolic one should fix it.

kmill commented 1 year ago

Oh right, I wasn't paying attention to which knot this was. I modified the test to have it compute the Alexander polynomial instead -- if I understood things correctly, this also removes the SnapPy dependency for this sort of test.

kmill commented 1 year ago

I've been doing sage -python -m spherogram.test to run the tests and didn't think to check that some docstring tests needed sage. Those start with sage: now.

kmill commented 1 year ago

@NathanDunfield I'm sure you're busy, but I just wanted to let you know that I expect the workflows to succeed now.

NathanDunfield commented 1 year ago

@kmill Thanks for the ping, I didn't realize this is ready for review.

I just merged a different PR that created some conflicts with yours. Could you resolve them? It should be OK just to go with your version as the changes in #39 should have been purely stylistic.

Also, while GH claims there are no merge conflicts with this PR, could you bring kennethleebaker:kmill_tangle_cat up to date with 3-manifolds:master? It's 21 commits behind, which means the CI passing may not mean as much as one hopes. Thanks!

kmill commented 1 year ago

@NathanDunfield Ok, I've finally got it merged with master!

NathanDunfield commented 1 year ago

Tests are passing, shall I merge it?

kmill commented 1 year ago

@NathanDunfield I think I'm happy with it to be merged.

I'll admit I have some reservations about the planar_isotopy file since there aren't so many tests, but I'm relatively confident I didn't break it.