Closed soehms closed 3 years ago
Branch: u/soehms/knotinfo
Tarball for KnotInfo
Attachment: knotinfo-20200713.tar.bz2.gz
New commits:
a79ddf5 | 30352: initial version providing just the basics |
Description changed:
---
+++
@@ -14,3 +14,15 @@
Many thanks to Allison Moore and Chuck Livingston for their kind permission to have this interface implemented and their offer to support us.
+
+
+Having checked out the ticket for the first time, you have to run
+
+```
+make SAGE_SPKG="sage-spkg -o" knotinfo build
+```
+
+in order to have the databases installed.
+
+Traball: https://github.com/sagemath/sage/files/ticket30352/knotinfo-20200813.tar.bz2.gz
+
Author: Sebastian Oehms
Description changed:
---
+++
@@ -19,7 +19,7 @@
Having checked out the ticket for the first time, you have to run
-make SAGE_SPKG="sage-spkg -o" knotinfo build +make SAGE_SPKG="sage-spkg -o" knotinfo-clean build
in order to have the databases installed.
Branch pushed to git repo; I updated commit sha1. New commits:
a8f1bfc | 30352: adding Gauss-code and symmetry type |
Description changed:
---
+++
@@ -22,7 +22,12 @@
make SAGE_SPKG="sage-spkg -o" knotinfo-clean build
-in order to have the databases installed.
+in order to have the databases installed. If you like to run all relevant doctests on the installation use:
+
+ +make SAGE_SPKG="sage-spkg -o" SAGE_CHECK="yes" knotinfo-clean build +
+
Traball: https://github.com/sagemath/sage/files/ticket30352/knotinfo-20200813.tar.bz2.gz
Branch pushed to git repo; I updated commit sha1. New commits:
1ec036c | 30352: add demo sample list of 20 links |
I add a sample of 20 links with about 20 of their properties as demonstration examples in order to have most of the doctests work on standard tests.
Branch pushed to git repo; I updated commit sha1. New commits:
a20c2f0 | build/pkgs/sage_sws2rst/src/test/Adding_Pictures_and_screenshots.sws |
60bae79 | Merge branch 'develop' of git://github.com/sagemath/sage into develop |
0c28e73 | Merge branch 'u/soehms/knotinfo' of git://trac.sagemath.org/sage into knotinfo_30352 |
092cd4c | 30352: improvements and documentation |
I do the following:
KnotInfoSeries
improving the access to the links.identify_knotinfo
) to the class Link
used in a new is_isotopic
method.injection
methods to inject names of links and series of links easily into the namespace.Now a state is reached where about a quarter of the more than 120 properties of links listed in the KnotInfo databases have conversions to Sage objects (whereas all of them can be accessed as string). Surely, useres will still miss conversion methods in the remaining cases. But, from my point of view the current set is sufficient for a start with that interface. More conversion methods can be added on demand. Therefore, I think this is ready for review.
Description changed:
---
+++
@@ -19,13 +19,13 @@
Having checked out the ticket for the first time, you have to run
-make SAGE_SPKG="sage-spkg -o" knotinfo-clean build +make SAGE_SPKG="sage-spkg -o" database_knotinfo-clean build
in order to have the databases installed. If you like to run all relevant doctests on the installation use:
-make SAGE_SPKG="sage-spkg -o" SAGE_CHECK="yes" knotinfo-clean build +make SAGE_SPKG="sage-spkg -o" SAGE_CHECK="yes" database_knotinfo-clean build
I feel like there should be something added to the features/databases.py
file to test that the package is installed rather than within the class itself. I think this is how we generally want to do things (but perhaps I am not the best person to ask about this).
I am not entirely convinced by name of the method identify_knotinfo
. I am thinking knotinfo
is easier to discover, but I am also not sure this is a good name too. You should add a doctest showing an example that is not covered by the database to show that works (not just when it cannot be uniquely determined).
Now for some minor things:
It would be good to limit things in the docstrings to ~80 characters per line.
Missing blankline:
def __init__(self, crossing_number, is_knot, is_alternating, name_unoriented=None):
r"""
Python constructor.
EXAMPLES::
sage: from sage.knots.knotinfo import KnotInfoSeries
sage: L6a = KnotInfoSeries(6, False, True); L6a
Series of links L6a
sage: TestSuite(L6a).run()
"""
You don't need to apologize :P
:
NotImplementedError: Sorry, this link cannot be uniquely determined
Also, error messages should start with a lowercase.
Do you need this import?
sage: from sage.databases.knotinfo_db import KnotInfoDataBase
- sage: from sage.env import SAGE_SHARE
sage: ki_db = KnotInfoDataBase()
sage: ki_db.filename.links
<KnotInfoFilename.links: ['https://linkinfo.sitehost.iu.edu/', 'linkinfo_data_complete']>
I don't see where it is used in the doctest.
The bullet points are overindented:
Briefly, these differences are:
- ``pd_notation`` -- KnotInfo: counter clockwise Sage: clockwise, see note in
:meth:`KnotInfoBase.link`
- ``homfly_polynomial`` -- KnotInfo: ``v`` Sage: `1/a`, see note in :meth:`KnotInfoBase.homfly_polynomial`.
- ``braid_notation`` -- This is used accordingly: The crossing of the braid generators are positive
in both systems. Here it is listed because there could arise confusion from the source where they are
taken from. There, the braid generators are assumed to have a negative crossing
(see definition 3 of `Gittings, T., "Minimum Braids: A Complete Invariant of Knots and Links <https://arxiv.org/abs/math/0401051>`__).
A trivial thing: Examples::
-> EXAMPLES::`
Replying to @tscrim:
Thank you for having a look at this, Travis! Also, many thanks for the organization of that great Sage Days!
I feel like there should be something added to the
features/databases.py
file to test that the package is installed rather than within the class itself. I think this is how we generally want to do things (but perhaps I am not the best person to ask about this).
Indeed, it seems so! I didn't notice that because only two of the databases did that and everything worked fine, so far. Surely, it would be no mistake to do it. So I will include that in the next commit.
I am not entirely convinced by name of the method
identify_knotinfo
. I am thinkingknotinfo
is easier to discover, but I am also not sure this is a good name too.
What about to_knotinfo
?
You should add a doctest showing an example that is not covered by the database to show that works (not just when it cannot be uniquely determined).
I will add one in the next commit and fix the other minor issues, as well.
Replying to @soehms:
Replying to @tscrim:
Thank you for having a look at this, Travis! Also, many thanks for the organization of that great Sage Days!
Thank you.
I am not entirely convinced by name of the method
identify_knotinfo
. I am thinkingknotinfo
is easier to discover, but I am also not sure this is a good name too.What about
to_knotinfo
?
Better, but it is not really returning a knotinfo()
object. I would be okay with get_knotinfo()
.
Replying to @tscrim:
Replying to @soehms:
What about
to_knotinfo
?Better, but it is not really returning a
knotinfo()
object. I would be okay withget_knotinfo()
.
It's okay for me, as well. The same concerning all your other suggestions, as you can see from the recent commit.
Note, that the SnapPy doctests don't work any more since #24483 introduced an incompatibility in 9.3.beta0 (create_ComplexNumber
did move to another module harming the import of snappy
).
Branch pushed to git repo; I updated commit sha1. New commits:
255a768 | 30352: once again |
please rebase your branch, it's red (ie does not merge/show)
Replying to @dimpase:
please rebase your branch, it's red (ie does not merge/show)
Thanks for the hint. On the one hand, I could merge the branch into 9.3.beta1 without any conflicts. On the other hand I got a difference in build/pkgs/sage_sws2rst/src/test/Adding_Pictures_and_screenshots.sws
where I have absolutely no idea how this came up:
.../src/test/Adding_Pictures_and_screenshots.sws | Bin 281797 -> 281795 bytes
I replaced this by the develop branch version (hoping that fixes the issue).
Replying to @soehms:
Replying to @dimpase:
please rebase your branch, it's red (ie does not merge/show)
Thanks for the hint. On the one hand, I could merge the branch into 9.3.beta1 without any conflicts. On the other hand I got a difference in
build/pkgs/sage_sws2rst/src/test/Adding_Pictures_and_screenshots.sws
where I have absolutely no idea how this came up:.../src/test/Adding_Pictures_and_screenshots.sws | Bin 281797 -> 281795 bytes
this is from #28838 - which you somehow changed, judging by the file size.
I replaced this by the develop branch version (hoping that fixes the issue).
Perhaps Matthias can have a look at this, qua the package structure.
It doesn't seem to be in the current diff according to trac.
A couple of suggestions:
Since Knotinfo uses yet another normalization for the HOMFLY polynomial, maybe we could add it to the options for Sage knots and links (maybe 'vz' could be a good name). It could simplify the code in the identification process.
Also, in the same identification, we don't really need to check for braid equality, braid conjugacy would be enough. Thanks to libbraiding we have a very fast is conjugated
method.
Replying to @miguelmarco:
Thanks a lot for your suggestions, Miguel!
A couple of suggestions:
Since Knotinfo uses yet another normalization for the HOMFLY polynomial, maybe we could add it to the options for Sage knots and links (maybe 'vz' could be a good name). It could simplify the code in the identification process.
My approach is to keep the sage conventions fixed and do all necessary adaptions in the KnotInfo wrapper. There, I add to some of the conversion methods a keyword argument sage_convention
which can be used to have the adaption performed.
So, are you suggesting to have vz
instead of that, or in addition? If you mean in addition, I don't see an essential simplification. If not, I think it could be confusing if for one method (homfly_polynomial
) the adaption is realized in class Link
whereas for others (jones_polynomial
, alexander_polynomial
) it would remain in KnotInfo
. Therefore, if we want to move the adaption to class Link
, I would prefer to do it in all cases.
Also, in the same identification, we don't really need to check for braid equality, braid conjugacy would be enough. Thanks to libbraiding we have a very fast
is conjugated
method.
Great! I missed that because I've tried is_conjugate
(omitting the final d
) which didn't terminate or results in an error from the Gap wrapper. That made me capitulate! Of course I will include that in the next commit.
BTW: As you predicted I encountered some other instances of the trivial diagram issue (#30346). In two cases you get errors (is_alternating
, khovanov_homology
) and in one case a wrong result (jones_polynomial
). Furthermore, in is_alternating
shouldn't we change this:
if not self.is_knot():
return False
into a NotImplementedError
?
Shall I create a ticket for each case, or one for all? This is now #31001!
So, are you suggesting to have
vz
instead of that, or in addition? If you mean in addition, I don't see an essential simplification. If not, I think it could be confusing if for one method (homfly_polynomial
) the adaption is realized in classLink
whereas for others (jones_polynomial
,alexander_polynomial
) it would remain inKnotInfo
. Therefore, if we want to move the adaption to classLink
, I would prefer to do it in all cases.
If this 'vz' normalization is used out there (and it is, as KnotInfo/LinkInfo uses it), it makes sense for Sage to acknowledge it, and provide the option of giving the HOMFLY polynomial in that notation. Users that are used to it might find it useful. And as a side effect, the code you are writing could be simpler.
The case of Alexander Polynomial might be different, since the ambiguity between different possible results comes from an ambiguous definition; instead of coming from different choice of notations.
Replying to @miguelmarco:
Users that are used to it might find it useful.
Now I know what you mean! I'll do that on a separate commit in order not to do too much at once. Currently, the defaults for var1
and var2
do not depend on the chosen normalization
(they are L
and M
regardless whether normalization=lm
or not). If you agree, I will take the occasion to have this more comfortable: More precisely, I would set the keyword defaults to None
and change None
according to the normalization
.
The case of Alexander Polynomial might be different, since the ambiguity between different possible results comes from an ambiguous definition; instead of coming from different choice of notations.
The ambiguity of definition could be dealt with a synchronization of normalization (which I realized up to sign). But you are right: in both other cases (Jones and Alexander polynomials) KnotInfo uses the same definition as we do. The differences come from the fact that KnotInfo avoids polynomials with negative or fractional exponents. I will leave the corresponding methods as they are, but maybe replace the keyword name sage_convention
by something more specific.
What about my question concerning is_alternating
?
Branch pushed to git repo; I updated commit sha1. New commits:
3a174d2 | 30352: use braid.is_conjugated and restructure get_knotinfo |
I do the following:
I use the braid method is_conjugated
in get_knotinfo
and is_isotopic
according to Miguel's suggestion. To deal with the case where the braid's parents are different I implemented a helper method _markov_move_cmp
for class Link
. Here the embedding into a common parent via Markov moves II is realized.
I restructure get_knotinfo
since I found some algorithmic leaks leading to irritating results. For example amphicheiral knots were detected as mirror image of their KnotInfo equivalent. Another source of confusion has been the fact that KnotInfo.L5a1_1.link()
has been identified as KnotInfo.L5a1_0
. The reason is that both are isotopic (see the note I add to get_knotinfo
). I prefer to raise an error in such a case and let the user apply the keyword unique=False
to obtain both of them.
knotinfo_matching_list
. One part of its contents is now in a new method lower_list
of class KnotInfoSeries
and the other part in a new method _knotinfo_matching_list
of class Link
. The latter method contains parts of the former get_knotinfo
in addition.I do some renaming and introduction of keywords in order to unify their usage, especially in the context of distinguishing between oriented and unoriented proper links.
I add __gt__
to class KnotInfoBase
in order to have sorted
work for lists of KnotInfo items.
The commit above contains the following changes:
_test_recover
for the classes KnotInfoBase
and KnotInfoSeries
in order to have TestSuite
check if the items of the KnotInfo databases can be recovered correctly (up to ambiguities) from the different conversions to Sage.get_knotinfo
and its auxillaries in order to improve its perfomance and remove bugs I detected using _test_recover
.cached_method
decorators.symmetry_type
and the value of determinant
for unknot).Running _test_recover
over the complete database (more than 28000 checks in sum) now takes about one and a half hour (on an i5). When I started to test this it didn't terminate after one night. At the moment there are still some failures (13 pairs of orientation mutants of proper links) all due to the current implementation of mirror_image
. I opened ticket #30997 to have this fixed.
BTW: By what reason is determinant
not implemented for proper links?
Branch pushed to git repo; I updated commit sha1. New commits:
febf374 | 39352: some pyflake fixes which got lost |
Replying to @soehms:
Running
_test_recover
over the complete database (more than 28000 checks in sum) now takes about one and a half hour (on an i5). When I started to test this it didn't terminate after one night. At the moment there are still some failures (13 pairs of orientation mutants of proper links) all due to the current implementation ofmirror_image
. I opened ticket #30997 to have this fixed.
Even if it takes 1.5 hours, that is still far too long as a default. You should use the tester._max_runs
of the tester to limit the number that gets run as a default. This at least makes it much more controllable for the doctests to limit it to a few seconds (which it then is marked as # long time
) with perhaps a < .5s run for the non-long tests.
Also, you will probably need to mark a lot of doctests as # optional - database_knotinfo
.
Replying to @tscrim:
Replying to @soehms:
Even if it takes 1.5 hours, that is still far too long as a default. You should use the
tester._max_runs
of the tester to limit the number that gets run as a default. This at least makes it much more controllable for the doctests to limit it to a few seconds (which it then is marked as# long time
) with perhaps a < .5s run for the non-long tests.
I didn't include the test of the complete database in the code. Only one over all proper alternating links with 5 crossings:
sage: %time TestSuite(KnotInfo.L5a1_0.series()).run()
CPU times: user 680 ms, sys: 52 ms, total: 732 ms
Wall time: 718 ms
I will mark this as long.
Thanks for your hint to tester._max_runs
. I will checkout how I can apply it.
Also, you will probably need to mark a lot of doctests as
# optional - database_knotinfo
.
No! All not maked tests run without the optional package by help of a small list of demonstration cases kept in static dictionaries.
Replying to @soehms:
Replying to @tscrim:
Replying to @soehms:
Even if it takes 1.5 hours, that is still far too long as a default. You should use the
tester._max_runs
of the tester to limit the number that gets run as a default. This at least makes it much more controllable for the doctests to limit it to a few seconds (which it then is marked as# long time
) with perhaps a < .5s run for the non-long tests.I didn't include the test of the complete database in the code. Only one over all proper alternating links with 5 crossings:
sage: %time TestSuite(KnotInfo.L5a1_0.series()).run() CPU times: user 680 ms, sys: 52 ms, total: 732 ms Wall time: 718 ms
I will mark this as long.
That is fine; it doesn't need to be marked as long.
Thanks for your hint to
tester._max_runs
. I will checkout how I can apply it.
It is very easy to add a TestSuite(KnotInfo).run()
test, which then will accidentally take a long time. Granted, it is on subsequent authors to avoid doing this, but a user might also do this, so this is now mostly a me being complete paranoid request.
Also, you will probably need to mark a lot of doctests as
# optional - database_knotinfo
.No! All not maked tests run without the optional package by help of a small list of demonstration cases kept in static dictionaries.
I see. I am not sure how I feel about that as I feel like it could hide stuff accidentally. I am inclined to keep it, but perhaps someone else can comment on this.
Replying to @tscrim:
It is very easy to add a
TestSuite(KnotInfo).run()
test, which then will accidentally take a long time. Granted, it is on subsequent authors to avoid doing this, but a user might also do this, so this is now mostly a me being complete paranoid request.
This danger does not exist in this special case:
sage: TestSuite(KnotInfo).run(verbose=True)
running ._test_new() . . . pass
running ._test_pickling() . . . pass
This is because KnotInfo
isn't inherited from SageObject
which is not possible for Enum
. Just the TestSuite
for KnotInfoSeries
use this method (indirectly). Obviously this TestSuite
construction causes some confusion. I will try to improve this according to your hints!
BTW: The test that run 1.5 h was this (no way to do that accidentally, but sorry that I didn't make this clear enought):
sage: from sage.misc.sage_unittest import instance_tester
....: from sage.knots.knotinfo import KnotInfo
....: tester = instance_tester(KnotInfo)
....: def test_all():
....: for L in KnotInfo:
....: try:
....: L._test_recover(tester=tester)
....: except AssertionError:
....: print(L)
....:
sage: %time test_all()
CPU times: user 1h 30min 45s, sys: 2.08 s, total: 1h 30min 47s
Wall time: 1h 30min 47s
I feel like it could hide stuff accidentally.
What stuff do you mean? Future changes in the real database for the first twenty links? I think changes in the values of the properties used can be ruled out and adding new properties wouldn't hurt.
The great advantage of these demo examples is that the basic functionality of this interface is permanently tested. So if someone introduces something into another library code that is incompatible, he becomes aware of it.
Replying to @soehms:
Replying to @tscrim:
It is very easy to add a
TestSuite(KnotInfo).run()
test, which then will accidentally take a long time. Granted, it is on subsequent authors to avoid doing this, but a user might also do this, so this is now mostly a me being complete paranoid request.This danger does not exist in this special case:
sage: TestSuite(KnotInfo).run(verbose=True) running ._test_new() . . . pass running ._test_pickling() . . . pass
This is because
KnotInfo
isn't inherited fromSageObject
which is not possible forEnum
. Just theTestSuite
forKnotInfoSeries
use this method (indirectly). Obviously thisTestSuite
construction causes some confusion. I will try to improve this according to your hints!
Ah, I think I now understand as I was misunderstanding what KnotInfoSeries
(which is what I meant before because of calling the _test_recover
; sorry for this error too) was holding. I thought it was holding onto the entire database.
BTW: The test that run 1.5 h was this (no way to do that accidentally, but sorry that I didn't make this clear enought):
sage: from sage.misc.sage_unittest import instance_tester ....: from sage.knots.knotinfo import KnotInfo ....: tester = instance_tester(KnotInfo) ....: def test_all(): ....: for L in KnotInfo: ....: try: ....: L._test_recover(tester=tester) ....: except AssertionError: ....: print(L) ....: sage: %time test_all() CPU times: user 1h 30min 45s, sys: 2.08 s, total: 1h 30min 47s Wall time: 1h 30min 47s
I would have expected the database itself to have a consistency check like this.
I feel like it could hide stuff accidentally.
What stuff do you mean? Future changes in the real database for the first twenty links? I think changes in the values of the properties used can be ruled out and adding new properties wouldn't hurt.
The link with the database somehow becomes broken, such as they change the name of a column. So the code breaks once you install the database. Granted, I think this is unlikely. Looking over the design a bit more, having a future developer should not naturally avoid the methods that differentiate between the two.
The great advantage of these demo examples is that the basic functionality of this interface is permanently tested. So if someone introduces something into another library code that is incompatible, he becomes aware of it.
I agree that it is an advantage to have it tested. Although I do believe it should not be done locally within Sage's library but with a more robust testing framework. Yet, I believe that the benefits here clearly outweigh the costs.
At the moment Sage offers just a small set of 250 named knots (
src/sage/knots/knot_table.py
) taken form the Rolfsen table. Proper named links aren't available at all.Nowadays, larger databases for knots and links are available at the Knot Atlas pages in RDF-format and at KnotInfo as XLS / XLSX -files. Since parsing of CSV files is already supported by Sage, this is a good start to produce a Sage packages from these files containing about 3000 knots and 4000 proper links together with a lot of their properties and invariants.
Such a package has a couple of advantages:
The aim of this ticket is to have the databases accessible in Sage together with conversion methods for the most important properties and invariants.
Many thanks to Allison Moore and Chuck Livingston for their kind permission to have this interface implemented and their offer to support us.
Having checked out the ticket for the first time, you have to run
in order to have the databases installed. If you like to run all relevant doctests on the installation use:
CC: @miguelmarco @mkoeppe @kiwifb
Component: algebraic topology
Keywords: knot, link
Author: Sebastian Oehms
Branch:
9cde996
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30352