Interface for Regina (3-manifold topology and normal surface theory)#40370
Interface for Regina (3-manifold topology and normal surface theory)#40370vbraun merged 22 commits intosagemath:developfrom
Conversation
|
Documentation preview for this PR (built with commit 4baf0d6; changes) is ready! 🎉 |
|
All relevant CI-tests passed including the ones that have the optional package install (Linux Incremental except the one for Fedora 41 which failed by unrelated reason). For example for Debian bookworm wie have: |
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
|
@tscrim thanks for reviewing! I will do your other points later on. |
|
Okay, just let me know when you're ready for me to have another look. |
I've now made the changes according to your suggestions. For the Feature class, I've enhanced the implementation by linking the |
|
@tscrim did you see my #40370 (comment) from July 8th? |
|
Sorry, it's been busy with organizing FPSAC this year. One thing I should have remembered earlier is you need to add/update the meson files. It's something we're now forced to do when adding new files (something to do with the building Sage). I will try to take another look soon. |
No problem!
I try to do so in the current commit,
Thanks! |
|
Just let me know when you've had time to consider the above suggestions. |
I already did that in July. Did I miss something? |
tscrim
left a comment
There was a problem hiding this comment.
Ah, sorry, I thought I had submitted my latest comments as GH was showing them for me.
src/sage/knots/link.py
Outdated
| @@ -2966,7 +3031,7 @@ def _isolated_components(self): | |||
| for j in G.connected_components(sort=False)] | |||
|
|
|||
| @cached_method | |||
There was a problem hiding this comment.
| @cached_method | |
| @cached_method(key=lambda s,v1,v2,n,a: (s,v1,v2,n)) |
We should ignore the algorithm when doing the caching. (I think this is the correct syntax; sorry if I am misremembering.)
There was a problem hiding this comment.
We should ignore the algorithm when doing the caching. (I think this is the correct syntax; sorry if I am misremembering.)
I'm not sure about that! For example, if you want to compare two algorithms, say
sage: all(K.homfly_polynomial() == K.homfly_polynomial(algorithm=regina.ALG_DEFAULT) for K in sample_list)
you will always get True, even if sample_list contains links where the algorithms don't match.
There was a problem hiding this comment.
It's possible to clear the cache, but the only reason to compare the algorithms would be for testing. Unless there is a convention difference?
There was a problem hiding this comment.
It's possible to clear the cache, but the only reason to compare the algorithms would be for testing. Unless there is a convention difference?
No there isn't. However, if there is a bug in the implementation of one of the algorithms (only appearing for specific knots, such as we had with the plot method, see #37587 (comment)), this can cause considerable confusion. Another example of confusion can arise when a user tries to figure out which of the algorithms is faster (especially if he doesn't know anything about cached_method).
There was a problem hiding this comment.
If someone is trying to test something between the algorithms, then they will see in the doctest how to clear the cache and do the comparison. Granted, in most cases, this won't matter because a user won't pass the algorithm, but the most likely scenario to me is a user passes the algorithm, then runs it again with the default (i.e., not passing the arg) that is different. However, the user simply wanted the result again and shouldn't need to wait for it to redo the computation (in a different way).
There was a problem hiding this comment.
I've now implemented your suggestion. Unfortunately, this resulted in a doctest failure in get_knotinfo:
sebastian@ThinkPadKlein:~/devel/sage$ sage -tp --optional=sage,regina src/sage/knots/link.py
Running doctests with ID 2025-08-21-18-31-37-5591f73b.
Git branch: add_regina
Git ref: 10.8.beta0-20-g151cc74569f-dirty
Running with SAGE_LOCAL='/home/sebastian/devel/sage/local' and SAGE_VENV='/home/sebastian/devel/sage/local/var/lib/sage/venv-python3.12.5'
Using --optional=regina,sage
...
Doctesting 1 file using 4 threads.
sage -t --warn-long 5.0 --random-seed=251566221754781793842429710826575491863 src/sage/knots/link.py
**********************************************************************
File "src/sage/knots/link.py", line 4315, in sage.knots.link.Link.get_knotinfo
Failed example:
k11mr.get_knotinfo()
Expected:
KnotInfo['K11n_82m']
Got:
KnotInfo['K11n_82']
**********************************************************************
1 item had failures:
1 of 56 in sage.knots.link.Link.get_knotinfo
[662 tests, 1 failure, 16.06s wall]
This happens because cached_method requires a hash for self, which forces the calculation of the braid:
> /home/sebastian/devel/sage/src/sage/knots/link.py(3033)<lambda>()->(Knot represen...y 11 crossings, None, None, 'vz')
-> @cached_method(key=lambda s,v1,v2,n,a: (s,v1,v2,n))
(Pdb)
--Call--
> /home/sebastian/devel/sage/src/sage/knots/link.py(643)__hash__()
-> def __hash__(self):
(Pdb)
> /home/sebastian/devel/sage/src/sage/knots/link.py(653)__hash__()
-> return hash(self.braid())
Now, get_knotinfo compares k11 = KnotInfo.K11n_82.link() with k11s = k11.mirror_image().reverse().mirror_image().reverse() to determine the symmetry type. Before the change, mirror_image and reverse processed the pd_code in each step. Therefore, k11 and k11s were identical. After the change, these methods process the braid in the second step, since the homfly polynomial has since been calculated. Unfortunately, the braid recorded in KnotInfo for KnotInfo.K11n_82 is not conjugate to the braid calculated by Sage. Therefore, get_knotinfo can no longer identify k11 with k11s.
I learned two things from this:
- There's a bug in
get_knotinfo, because it should have raised an error instead of returning the wrong symmetry type. - Symmetry type detection can be improved by additionally comparing with the braid calculated by Sage.
I think the first point requires a separate PR, which I'll open later. I'll add the second point to the current commit. This fixes the doctest.
There was a problem hiding this comment.
So everything for this PR is now done? At least, my understanding is that the change I proposed only uncovered this and did not cause it (or at least it shouldn't). Sorry I'm not quite following the details.
There was a problem hiding this comment.
So everything for this PR is now done?
Yes!
At least, my understanding is that the change I proposed only uncovered this and did not cause it (or at least it shouldn't).
Exactly!
FYI: I'm on vacation for the next two weeks, so I won't be responding very often.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
|
Caution: The current commit introduced build errors (see for example this in this CI-run). I will look at them later on! |
Done in 151cc74. |
tscrim
left a comment
There was a problem hiding this comment.
Alright, thanks. Let's get this in.
PS - Enjoy your vacation.
Thanks! |
This PR is intended to resolve the old Trac ticket #17077 (now a GitHub issue). This branch is based on the branch of Trac ticket #31456, which addressed the implementation of the optional
SPKG.In the latter ticket it was noted that the package is not compatible with 32-bit systems and is therefore considered experimental (see #31456 (comment)). According to #31456 (comment), this should now be resolved. However, I haven't verified this yet. Are there still CI runs for this? Is this still a policy?
If this issue turns out to have resisted, I will change the type to experimental.
This PR initiates the integration of Regina via the Sage interface framework. Most of the basics should be included, but given Regina's broad functionality, this PR is still selective focusing on some applications of knot theory. However, future extensions should now be fairly straightforward. Explicitly it starts with the following specific conversions:
📝 Checklist
⌛ Dependencies