-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Knot Theory as a part of GSoC 2014. #17030
Comments
Branch: u/amitjamadagni/knots |
Commit: |
Author: amitjamadagni, mmarco |
comment:5
Things to do:
|
Reviewer: mmarco |
comment:8
I am not sure if it is a good idea that i would be the reviewer, since i have been involved in tge coding too. Besides, there is some polishing needed still and it would go faster if i directly work on it. jhpamieri, do you feel able to do the revieweing? If yes, i will start working in the corrections. Otherwise, i will stay away from changing the code and just act as reviewer. |
comment:9
I get the following wrror when compiling: running install_lib Is it a problem with the commit? Over which branch is it based? |
comment:10
Replying to @miguelmarco:
I pushed the branch from github (the branch week15) onto the trac ticket. Let me try adding all.py and commit once again. |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Changed reviewer from mmarco to Miguel Marco |
Changed author from amitjamadagni, mmarco to Amit Jamadagni, Miguel Marco |
comment:12
It looks like a bad resolution to a merge, but it's a simple enough fix. Also on a quick look through:
pd_error = _pd_check_(input_)
if pd_error == True:
pass
elif pd_error == False:
pass into (the pythonic) if _pd_check_(input_):
pass
else:
pass
I'll make a more detailed pass through the code after these are addressed. I also don't know how much of the math I'll be able to review as I've studied a little knot theory, but possibly not enough to check everything. Although Miguel could count as the reviewer on that front. I think this will be a great addition to Sage, all it needs a little polish. Best, Travis |
comment:13
Also please respect http://www.sagemath.org/doc/developer/coding_basics.html#headings-of-sage-library-code-files and add your new files to the reference manual (have a look at the files in Even better, change
to
and change the last line of |
comment:14
Instead of returning a string in |
comment:15
Replying to @jdemeyer:
I am currently having few issues with pushing commits onto trac, once I am done with the setup I will be sending in the commits. Thanks for guiding me and sorry for missing on the rules. |
comment:16
Yeah, I think it would be really wise for such a new component and with a lot of technicalities for determining e.g. whether a given link is a knot to have a couple actual knot theorists review it, at least by checking the output in lots of cases. That may take some cold-calling, though perhaps a sage-devel email will turn up some all by itself, and there are Sage-friendly knot folks out there - though I don't know if they know how to review a branch. |
comment:17
I was planning on making a package with the database from the knot atlas, and use it to run automated tests, computing the invariants for the knots and links there. The problem with that approach is that a) It needs some nontrivial work to be implemented, and b) it restricts to knots and links presented im spcially "nice" ways, so we could miss some bugs if they show up only when some strange representations are used. Anyways, it would be a nice addition by itself, so i will try to work on it in the following weeks. In the meantime, and since there are several other reviewers in the coding style part, i will just focus on checking the mathematical correctness of the methods. |
comment:18
There is something working wrong in the following case:
It gets an error when trying to get the connected components, which in turn tries to convert it to braid (which is probably not a good idea, it would be wiser to compute the components directly from the PD_code, since we might me loosing something in the conversion PD<->braid). In any case, this example fails in the conversion to braid:
Please go through it and debug. |
comment:19
That's a really excellent idea. It's always good to have two ways to check information anyway. Ideally one could have a prototype of that available in time to help test this ticket as well. |
comment:190
Should we set this back to "positive review"? I don't plan to make any more changes. |
Changed reviewer from Miguel Marco, Karl-Dieter Crisman, Frédéric Chapoton, Travis Scrimshaw, Søren Fuglede Jørgensen to Miguel Marco, Karl-Dieter Crisman, Frédéric Chapoton, Travis Scrimshaw, Søren Fuglede Jørgensen, John Palmieri |
Changed branch from public/ticket/17030 to |
comment:193
See #20315 for a followup. |
Changed commit from |
comment:194
Replying to @jhpalmieri:
In more detail, whoever is familiar with the plotting code should review the changes there. |
comment:195
Follow-up ticket at #25050: allow braid computation for more links. |
Who can remember ... ?I'm interested in the background of a decision about the |
@amitjamadagni , @miguelmarco , @tscrim , @kcrisman , @jhpalmieri , @fchapoton , @fuglede I wonder if notification works for migrated participants of a migrated Trac ticket. Did you see my #17030 (comment) above from last Tuesday? |
I don't see it on my GH notifications, just this one. I guess you have to "subscribe" to the ticket (whether universally or in particular), and that doesn't seem to have happened. Could be worth talking about on sage-devel if it hasn't already shown up as one of the tickets on the trac-to-github repo (and I'm not sure what the default should be, if any). |
And for what it's worth, I do not remember the story behind the clock/counter (non?)decision 😄 |
I didn't see it on my notifications either, just this one. About the decission, IIRC we followed snappy convention. |
Thank you both. Miguel, currently SnapPy follows the counter clockwise convention, too:
This is compensated by the transition method The question about the convention has been arisen by Chuck Livingston. He has been very confused by it, as I was when I implemented the KnotInfo interface. But besides this, he is very impressed with Sage Knot Theory and hopeful that it will become the standard platform for knot theory researchers. Maybe, we should think about a change of the |
I see, I guess the origin comes from that difference in the braid representation: I must have assumed that snappy uses the same convention for braids, and inferred from there the clockwise convention for links. Since there is no real reason to keep that convention (aside from not breaking existing code that relies on it), i have no objection to change it... being very careful with the issue of not breaking things. |
The doctests should (in principle) be sufficient to catch if anything breaks. Although there will be no way to ensure a transition period where downstream code using this convention explicitly would not break. I think we just have to take our medicine here and switch the conventions with a big warning in our release notes about it, possibly also in the PD code documentation itself. |
I had the backward confusion when I implemented the KnotInfo interface. First I thought the reflection of knots was due to a difference in the braid convention since the referenced paper on KnotInfo had the inverse convention. But than I realized, that the braid convention in KnotInfo is the same as ours despite of the reference. |
I completely agree. But, we should try to do the switch simultaneously with an adaption in the SnapPy
@NathanDunfield will this be possible? I know, this does not prevent users to have incompatible versions of Sage and SnapPy. But if in both there is an note about the switch of convention and which versions are compatible it would be acceptable. |
@miguelmarco For what it's worth, SnapPy changed its convention for braids with the 3.0 release. Positive braid generators now give positive crossings (we used to follow the convention in Birman's classic book, which is the reverse). @soehms I think the best solution would be for SnapPy to test for the version of Sage and then convert appropriately. There's no way to avoid the problem of (old SnapPy, new Sage) but the other three combinations would all work correctly. |
@NathanDunfield Of course, the |
That would be great. But an additional note in the docstring would be nice for the surprise when the user notices the change for the first time. |
Since there are no objections against the change here, I ask on sage-devel for opinions. If there will be no other objections, I will open an issue / PR in one of the next weeks. |
Now, there is PR #35665 implementing the switch of convention being ready for review. |
We provide a basic implementation of knots and links such as the Seifert matrix, Jones and Alexander polynomials.
CC: @miguelmarco @jhpalmieri @tscrim @fuglede
Component: algebraic topology
Author: Amit Jamadagni, Miguel Marco
Branch:
207d033
Reviewer: Miguel Marco, Karl-Dieter Crisman, Frédéric Chapoton, Travis Scrimshaw, Søren Fuglede Jørgensen, John Palmieri
Issue created by migration from https://trac.sagemath.org/ticket/17030
The text was updated successfully, but these errors were encountered: