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

Adapt the KnotInfo interface to new Khovanov polynomial data (merged) #35800

Closed
wants to merge 4 commits into from

Conversation

soehms
Copy link
Member

@soehms soehms commented Jun 20, 2023

CAUTION

Although this PR is not marked as merged it has been merged into 10.1.beta6

Note, that Files changed 0! For more information see #35800 (comment).

📚 Description

A note in the current Khovanov polynomial method of the KnotInfo interface says:

The data used here were calculated with the program KhoHo. They are no longer visible on the website as of October 30, 2022. Instead, data computed with KnotJob are now displayed ... This interface will be adapted to the changes in an upcoming release.

The goal of this PR is the announced adaptation. Accordingly the PR lets Sage point to the current version 2023.6.1 of the Python wrapper of the database which contains the new columns.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@soehms soehms marked this pull request as draft June 21, 2023 16:58
@soehms soehms marked this pull request as ready for review June 21, 2023 21:47
@soehms soehms force-pushed the khovanov_knotjob_adaption branch from c32a834 to 9874e64 Compare June 21, 2023 21:57
@soehms
Copy link
Member Author

soehms commented Jul 4, 2023

I don't know why all GitHub bots except the one building the documentation have been cancelled? Anyway, the changes of the branch do only effect src/sage/knots/knotinfo.py and here all tests are fine.

@soehms soehms requested a review from tscrim July 4, 2023 16:25
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few very minor things.

src/sage/knots/knotinfo.py Outdated Show resolved Hide resolved
src/sage/knots/knotinfo.py Outdated Show resolved Hide resolved
src/sage/knots/knotinfo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM.

@soehms
Copy link
Member Author

soehms commented Jul 6, 2023

Thank you. LGTM.

Thank you, as well!

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 10, 2023

@vbraun: strangely this PR is still open.

@soehms
Copy link
Member Author

soehms commented Jul 11, 2023

@vbraun: strangely this PR is still open.

It seems that indeed commit 3291f4d (which just removes a blank-line) has not been merged. Probably this keeps it open.

@vbraun
Copy link
Member

vbraun commented Jul 19, 2023

Merge conflict

@soehms
Copy link
Member Author

soehms commented Jul 20, 2023

Merge conflict

@vbraun there is some mystery with this PR. It is already merged into 10.1.beta6 but just one commit (3291f4d) remains off (which just removes a blank line, see #35800 (comment) and #35172 (comment) at BTW).

Now, if I merge this branch with develop and try to push it back, git says Everything up-to-date. So what can I do?

Note my review comment: #35800 (comment): With the missing commit there has been something irregular from the beginning: It was not visible on the PR (at least for a couple of hours) after I've pushed it. Maybe this is related to the problem now!

@github-actions
Copy link

Documentation preview for this PR (built with commit ab0a42e; changes) is ready! 🎉

@soehms soehms closed this Jul 23, 2023
@soehms soehms reopened this Jul 23, 2023
@soehms soehms changed the title Adapt the KnotInfo interface to new Khovanov polynomial data Adapt the KnotInfo interface to new Khovanov polynomial data (merged) Jul 23, 2023
@soehms soehms closed this Jul 23, 2023
@soehms soehms deleted the khovanov_knotjob_adaption branch July 23, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants