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

Changing the knot theory PD-code convention #35665

Merged
merged 2 commits into from
Jun 3, 2023

Conversation

soehms
Copy link
Member

@soehms soehms commented May 22, 2023

📚 Description

According to a discussion following this comment of last February in #17030 this PR implements the transition of the PD-code convention from clockwise to anti-clockwise.

📝 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

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: ff44af3

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.

@miguelmarco I am assuming from the positive review you also meant to approve this.

Also LGTM.

@NathanDunfield
Copy link
Contributor

What version of Sage is this likely to appear in? We're about to do a release of SnapPy and want correctly convert from Sage's Link to SnapPy's depending on the Sage version.

@tscrim
Copy link
Collaborator

tscrim commented May 23, 2023

@NathanDunfield It almost certainly should get into 10.1, but I couldn't say which beta version (although there is a reasonable possibility it is the next one).

@soehms
Copy link
Member Author

soehms commented May 23, 2023

@miguelmarco I am assuming from the positive review you also meant to approve this.

Also LGTM.

Many thanks to you both!

@soehms
Copy link
Member Author

soehms commented May 23, 2023

@NathanDunfield It almost certainly should get into 10.1, but I couldn't say which beta version (although there is a reasonable possibility it is the next one).

I was hoping to do it for version 10.0, but unfortunately too much was preventing me from achieving that goal.

@NathanDunfield
Copy link
Contributor

@soehms @tscrim: Thanks for the clarification. I have future-proofed SnapPy for this change in 3-manifolds/Spherogram@d0ce3de3253

@soehms
Copy link
Member Author

soehms commented May 23, 2023

@soehms @tscrim: Thanks for the clarification. I have future-proofed SnapPy for this change in 3-manifolds/Spherogram@d0ce3de3253

Great! Do you already know when it will be released? I think we should upgrade the SPKG afterwards to the new version, as well.

@NathanDunfield
Copy link
Contributor

@soehms @tscrim: Thanks for the clarification. I have future-proofed SnapPy for this change in 3-manifolds/Spherogram@d0ce3de3253
Great! Do you already know when it will be released? I think we should upgrade the SPKG afterwards to the new version, as well.

SnapPy 3.1 with Spherogram 2.2 which supports this PD change in Sage was released yesterday on PyPI and Github.

@soehms
Copy link
Member Author

soehms commented May 25, 2023

@soehms @tscrim: Thanks for the clarification. I have future-proofed SnapPy for this change in 3-manifolds/Spherogram@d0ce3de3253
Great! Do you already know when it will be released? I think we should upgrade the SPKG afterwards to the new version, as well.

SnapPy 3.1 with Spherogram 2.2 which supports this PD change in Sage was released yesterday on PyPI and Github.

In fact, this was a last minute opportunity. Thanks very much!

@NathanDunfield
Copy link
Contributor

SnapPy 3.1 with Spherogram 2.2 which supports this PD change in Sage was released yesterday on PyPI and Github.

In fact, this was a last minute opportunity. Thanks very much!

You're very welcome.

@vbraun
Copy link
Member

vbraun commented May 28, 2023

Merge conflict

@vbraun
Copy link
Member

vbraun commented May 28, 2023

Sorry, ticket number off by one ;)

@vbraun vbraun merged commit f3c965f into sagemath:develop Jun 3, 2023
@soehms
Copy link
Member Author

soehms commented Jun 11, 2023

@soehms @tscrim: Thanks for the clarification. I have future-proofed SnapPy for this change in 3-manifolds/Spherogram@d0ce3de3253
Great! Do you already know when it will be released? I think we should upgrade the SPKG afterwards to the new version, as well.

SnapPy 3.1 with Spherogram 2.2 which supports this PD change in Sage was released yesterday on PyPI and Github.

@NathanDunfield, it seems that this commit did not find its way into the Spherogram 2.2 release:

┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.1.beta3, Release Date: 2023-06-11              │
│ Using Python 3.8.10. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛

sage: import spherogram
sage: spherogram.version()
'2.2'
sage: L = spherogram.Link('K10n11')
sage: L.PD_code()
[(2, 0, 3, 19),
 (0, 6, 1, 5),
...
 (18, 14, 19, 13)]
sage: L.sage_link().pd_code()
[[3, 20, 4, 1],
 [1, 6, 2, 7],
...
 [19, 14, 20, 15]]
sage:
sage: spherogram.Link.sage_link??
Signature: spherogram.Link.sage_link(self)
Docstring:
   Convert to a SageMath Knot or Link:

      sage: L = Link('K10n11')   # Spherogram link
      sage: K = L.sage_link(); K
      Knot represented by 10 crossings
...
        if SageKnot is None:
            raise ValueError('Your SageMath does not seem to have a native link type')
        sage_type = SageKnot if len(self.link_components) == 1 else SageLink
        # Sage's PD_code lists strands *clockwise* not our *anticlockwise*.
        code = [[x[0], x[3], x[2], x[1]] for x in self.PD_code(min_strand_index=1)]
        return sage_type(code)
File:      ~/devel/sage/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/spherogram/links/invariants.py
Type:      function

@NathanDunfield
Copy link
Contributor

NathanDunfield commented Jun 11, 2023

Sorry, it seems I only fixed one of the directions (sage -> spherogram) and not the other (spherogram -> sage). I believe I have fixed sage_link here. Could you please test it out before I release it as 2.2.1?

@soehms soehms deleted the change_pd_convention branch June 11, 2023 18:26
@soehms
Copy link
Member Author

soehms commented Jun 11, 2023

Sorry, it seems I only fixed one of the directions (sage -> spherogram) and not the other (spherogram -> sage).

No problem! Indeed, your new code was not contained in the former commit.

I believe I have fixed sage_link here. Could you please test it out before I release it as 2.2.1?

It works! Many Thanks!

@NathanDunfield
Copy link
Contributor

NathanDunfield commented Jun 11, 2023

@soehms As a final check, could you do the following with Sage 10.1.beta3:

sage -python -m spherogram.test

You should see:

[...]
All doctests:
   0 failures out of 458 tests.
[...]
OK

Thanks.

@soehms
Copy link
Member Author

soehms commented Jun 11, 2023

@soehms As a final check, could you do the following with Sage 10.1.beta3:

All tests passed, too!

@NathanDunfield
Copy link
Contributor

Thanks, I've released Spherogram 2.2.1 on PyPI.

@soehms
Copy link
Member Author

soehms commented Jun 12, 2023

Thanks, I've released Spherogram 2.2.1 on PyPI.

Thanks! Now, some optional doctests concerning SnapPy needs work on Sage side. I will open a corresponding follow up PR soon.

@soehms
Copy link
Member Author

soehms commented Jun 12, 2023

I will open a corresponding follow up PR soon.

This is #35755, now.

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.

6 participants