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

Interface to the KnotInfo and LinkInfo databases #30352

Closed
soehms opened this issue Aug 14, 2020 · 91 comments
Closed

Interface to the KnotInfo and LinkInfo databases #30352

soehms opened this issue Aug 14, 2020 · 91 comments

Comments

@soehms
Copy link
Member

soehms commented Aug 14, 2020

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:

  1. Perform cross-checks for about 7000 links of alternative implementations of certain methods.
  2. Do cross-checks against results listed in the KontInfo database.
  3. Provide properties for these links that are not provided by Sage, yet.
  4. Implement a link identification method for our link class (like KnotFinder).
  5. Launch webpages containing additional information for a link or alternate graphical representations.

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

./configure --enable-download-from-upstream-url
sage -i database_knotinfo

in order to have the databases installed. If you like to run all relevant doctests on the installation use:

sage -i -c database_knotinfo

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

@soehms soehms added this to the sage-9.3 milestone Aug 14, 2020
@soehms
Copy link
Member Author

soehms commented Aug 14, 2020

Branch: u/soehms/knotinfo

@soehms
Copy link
Member Author

soehms commented Aug 14, 2020

Tarball for KnotInfo

@soehms
Copy link
Member Author

soehms commented Aug 14, 2020

Commit: a79ddf5

@soehms
Copy link
Member Author

soehms commented Aug 14, 2020

comment:2

Attachment: knotinfo-20200713.tar.bz2.gz

New commits:

a79ddf530352: initial version providing just the basics

@soehms

This comment has been minimized.

@soehms
Copy link
Member Author

soehms commented Aug 14, 2020

Author: Sebastian Oehms

@soehms

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

Changed commit from a79ddf5 to a8f1bfc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

a8f1bfc30352: adding Gauss-code and symmetry type

@soehms

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

1ec036c30352: add demo sample list of 20 links

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2020

Changed commit from a8f1bfc to 1ec036c

@soehms
Copy link
Member Author

soehms commented Aug 23, 2020

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

Changed commit from 1ec036c to 092cd4c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 1, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

a20c2f0build/pkgs/sage_sws2rst/src/test/Adding_Pictures_and_screenshots.sws
60bae79Merge branch 'develop' of git://github.com/sagemath/sage into develop
0c28e73Merge branch 'u/soehms/knotinfo' of git://trac.sagemath.org/sage into knotinfo_30352
092cd4c30352: improvements and documentation

@soehms
Copy link
Member Author

soehms commented Nov 1, 2020

comment:10

I do the following:

  1. Add a new class KnotInfoSeries improving the access to the links.
  2. Add an link identification method (identify_knotinfo) to the class Link used in a new is_isotopic method.
  3. Add some more conversion methods for link invariants to Sage objects.
  4. Add injection methods to inject names of links and series of links easily into the namespace.
  5. Add conversion to SnapPy links.
  6. Some improvements in code sructure.
  7. Dokumentation.

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.

@soehms

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Nov 2, 2020

comment:11

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::`

@soehms
Copy link
Member Author

soehms commented Nov 3, 2020

comment:12

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 thinking knotinfo 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.

@tscrim
Copy link
Collaborator

tscrim commented Nov 4, 2020

comment:13

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 thinking knotinfo 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().

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

acddc12Merge branch 'develop' of git://github.com/sagemath/sage into develop
b5c0b1aMerge branch 'u/soehms/knotinfo' of git://trac.sagemath.org/sage into knotinfo_30352
654a20f30352: corrections according to review

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2020

Changed commit from 092cd4c to 654a20f

@soehms
Copy link
Member Author

soehms commented Nov 7, 2020

comment:15

Replying to @tscrim:

Replying to @soehms:

What about to_knotinfo?

Better, but it is not really returning a knotinfo() object. I would be okay with get_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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2020

Changed commit from 654a20f to 255a768

@soehms
Copy link
Member Author

soehms commented Feb 1, 2021

comment:59

I do the following:

  1. Changes which I have announced in comments 49, 50 and 56 according to review.
  2. Upgrade to a new tarball since one of the both Excel files has changed (three additional columns and some whitespace erasing).
  3. I add 2 more conversion methods (three_genus and signature) of link properties which have been of interest, recently (knot genus outputs wrong value for 5_2 #31188).
  4. One fix, since there was still a single link (L10a171_1_1_0) that caused TestSuite (with option max_samples=infinity) fail for class KnotInfoDatabase.
  5. I put KnotInfo and KnotInfoSeries into the global namespace in case the database is installed.

@soehms

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 4, 2021

comment:61

Some suggestions for the upstream repository (in the direction of #30914):

  • Call it database_knotinfo, not sagemath_knotinfo -- it will be useful to a broader community (Python)
  • It's redundant to put versioned tarballs in a git repository - put instead the unpacked tarball there and have git take care of versioning
  • When done, I can send you a pull request that turns this repository into a pip-installable package

@soehms
Copy link
Member Author

soehms commented Feb 6, 2021

comment:62

Replying to @mkoeppe:

Some suggestions for the upstream repository (in the direction of #30914):

  • Call it database_knotinfo, not sagemath_knotinfo -- it will be useful to a broader community (Python)
  • It's redundant to put versioned tarballs in a git repository - put instead the unpacked tarball there and have git take care of versioning
  • When done, I can send you a pull request that turns this repository into a pip-installable package

Sounds good! I hope the new repository is as expected. Please don't hesitate to make any changes you think are necessary! Many Thanks!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 15, 2021

Changed commit from 5844cae to 6e5e1e5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 15, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

1721a44Merge branch 'u/soehms/knotinfo' of trac.sagemath.org:sage into knotinfo_30352
6e5e1e5adaption to new beta version and some typo and style fixes

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 24, 2021

comment:64

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 15, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

66a555bMerge branch 'u/soehms/knotinfo' of trac.sagemath.org:sage into knotinfo_30352
9cde99630352: installation via PyPI

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 15, 2021

Changed commit from 6e5e1e5 to 9cde996

@soehms
Copy link
Member Author

soehms commented May 15, 2021

comment:66

Replying to @mkoeppe:

  • When done, I can send you a pull request that turns this repository into a pip-installable package

I've tried it now on my own and at least it is working. But since I needed a lot of trial error cycles I would appreciate it if you could have a look at it.

The current commit here concerns the adaption to the pip-installable package. Furthermore, it contains adaptations to SnapPy 3.0.1 and an addition of some verbose messages to is_isotopic (following a suggestion of knot theorists from the University of Regensburg).

@soehms

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented May 21, 2021

comment:68

I don't think it is necessary to cache homfly_polynomial() as all of the key computational aspects are cached and so you don't cache the "same" object even though someone changed the variable name.

Other than that, I am happy with the current state of things. Does anyone else have any comments or suggestions?

@mkoeppe
Copy link
Contributor

mkoeppe commented May 21, 2021

comment:69

Looks great to me!

@mkoeppe
Copy link
Contributor

mkoeppe commented May 21, 2021

Reviewer: Matthias Koeppe

@soehms
Copy link
Member Author

soehms commented May 21, 2021

comment:70

I don't think it is necessary to cache homfly_polynomial() as all of the key computational aspects are cached and so you don't cache the "same" object even though someone changed the variable name.

I agree that this is not that effective. In general, my consideration concerning caching was that with the database available you easily can have hundreds or thousands of invocations of any method. Anyway, I think that is nothing that could hurt, and thus would keep it for a start.

Many thanks to everyone who helped to have this interface realized!

@vbraun
Copy link
Member

vbraun commented Jun 6, 2021

Changed branch from u/soehms/knotinfo to 9cde996

@kiwifb
Copy link
Member

kiwifb commented Jun 7, 2021

comment:72

Follow up at #31921.

@kiwifb
Copy link
Member

kiwifb commented Jun 7, 2021

Changed commit from 9cde996 to none

@soehms
Copy link
Member Author

soehms commented Oct 26, 2021

comment:73

Another follow up at #32760.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants