Extend the method get_knotinfo of the Link class to cover non-prime knots #38254
Conversation
|
Documentation preview for this PR (built with commit cfe19f9; changes) is ready! 🎉 |
|
All bots green! |
Test
|
There are much more cases with the above problem: I didn't notice them before because they were better hidden by the original At the moment it is unclear to me whether there is incorrect data in the database or whether the Sage method |
@tscrim, I need your help here. According to your suggestion on July 15th I added an inheritance to It took me some time to find the commit which causes this. It was authored by @enriqueartal in connection with #38155 and changes the inheritance order in At the moment I have no idea to fix that properly. Of course one option would be to remove the inheritance from |
|
Hmm…interesting. I am a bit surprised by this, but it might be a good thing that this is showing up. I might need to look at it much more closely later. One thing to do would be to put the |
Many thanks to have a look at it! I have experimented with your suggestions. If I combine your first point with the second, I get: If I combine your first point with the third I get: |
|
Sorry for being slow to respond (1 week old baby and public holidays). I need to experiment with this to figure things out. I hope to do so this week |
That's really great news, Travis! Congratulations! My kids are already adults and live more than 200 km away (both happen to be in Munich). But I still remember very well how radically their birth changed life (especially in the first case). How about this plan:
This would remove the time pressure and reduce the likelihood of further merge conflicts. What do you think? |
Thank you. Indeed, although this is the second, so we are a bit more seasoned parents.
-1 Let's get this right the first time. I won't need much time, but I just need to play around a bit see what is happening. |
Yes, the second should be more relaxed (at least in the beginning, before they have their own personality).
No problem! |
|
This is actually independent of the change in #38155, which somehow just made the bug appear. There are two issues:
Here is a diff with the changes that I was able to get the code work: diff --git a/src/sage/knots/free_knotinfo_monoid.py b/src/sage/knots/free_knotinfo_monoid.py
index acf286fba0b..c8301a4d138 100644
--- a/src/sage/knots/free_knotinfo_monoid.py
+++ b/src/sage/knots/free_knotinfo_monoid.py
@@ -27,7 +27,7 @@ AUTHORS:
from sage.knots.knotinfo import SymmetryMutant
from sage.monoids.indexed_free_monoid import IndexedFreeAbelianMonoid, IndexedFreeAbelianMonoidElement
from sage.misc.cachefunc import cached_method
-from sage.misc.fast_methods import Singleton
+from sage.structure.unique_representation import UniqueRepresentation
class FreeKnotInfoMonoidElement(IndexedFreeAbelianMonoidElement):
@@ -89,7 +89,7 @@ class FreeKnotInfoMonoidElement(IndexedFreeAbelianMonoidElement):
return [P._index_dict[w] for w in wl]
-class FreeKnotInfoMonoid(IndexedFreeAbelianMonoid, Singleton):
+class FreeKnotInfoMonoid(IndexedFreeAbelianMonoid):
Element = FreeKnotInfoMonoidElement
@@ -106,12 +106,15 @@ class FreeKnotInfoMonoid(IndexedFreeAbelianMonoid, Singleton):
sage: FreeKnotInfoMonoid(5)
Free abelian monoid of knots with at most 5 crossings
"""
- return super().__classcall__(cls, max_crossing_number, prefix=prefix, **kwds)
+ if not prefix:
+ prefix = 'KnotInfo'
+ # We skip the IndexedMonoid__classcall__
+ return UniqueRepresentation.__classcall__(cls, max_crossing_number, prefix=prefix, **kwds)
def __init__(self, max_crossing_number, category=None, prefix=None, **kwds):
r"""
- Init this monoid with generators belonging to prime knots with at most
- ``max_crossing_number`` crossings.
+ Initialize ``self`` with generators belonging to prime knots with
+ at most ``max_crossing_number`` crossings.
TESTS:
@@ -125,8 +128,6 @@ class FreeKnotInfoMonoid(IndexedFreeAbelianMonoid, Singleton):
self._set_index_dictionary(max_crossing_number=max_crossing_number)
from sage.sets.finite_enumerated_set import FiniteEnumeratedSet
indices = FiniteEnumeratedSet(self._index_dict)
- if not prefix or prefix == 'F':
- prefix = 'KnotInfo'
super().__init__(indices, prefix)
def _from_knotinfo(self, knotinfo, symmetry_mutant):Note the normalization of the |
Thank you for your suggestion and the very clear explanations! Locally all tests under |
|
Green bot => setting positive review. |
Thanks! |
… cover non-prime knots
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
This PR implements a task from the following code-comment in `link.py`:
> # ToDo: extension to non prime links in which case an element
of the monoid
> # over :class:`KnotInfo` should be returned
In connection with this I switch the output of `get_knotinfo` to
elements of the new class `FreeKnotInfoMonoid` for all knots (prime or
not) For multi-component links I keep the output as it is, since the
`FreeKnotInfoMonoid` does not make sence here.
Another thing changes: Currently, if the `unique=False` option is used
symmetry mutants may be listed separately, even though they are known to
be isotopic because their symmetry type is reversible or amphicheiral.
Now, if symmetry mutants are known to be isotopic because of their
symmetry type, just the minimal of them is listed (in the order of the
enum).
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
URL: sagemath#38254
Reported by: Sebastian Oehms
Reviewer(s): Sebastian Oehms, Travis Scrimshaw
This PR implements a task from the following code-comment in
link.py:In connection with this I switch the output of
get_knotinfoto elements of the new classFreeKnotInfoMonoidfor all knots (prime or not) For multi-component links I keep the output as it is, since theFreeKnotInfoMonoiddoes not make sence here.Another thing changes: Currently, if the
unique=Falseoption is used symmetry mutants may be listed separately, even though they are known to be isotopic because their symmetry type is reversible or amphicheiral. Now, if symmetry mutants are known to be isotopic because of their symmetry type, just the minimal of them is listed (in the order of the enum).📝 Checklist
⌛ Dependencies