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

Fusion ring refactoring #35501

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

willieab
Copy link

📚 Description

Re-factoring the FusionRing code to expand its functionality from dealing exclusively with Verlinde algebras. Unifies VerlindeAlgebra and FusionDouble class.

📝 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

…r a couple issues with get_order and FusionRingFromWCR.__classcall__. Need some documentation/explanation in FusionRing constructor
…ith original FusionRing. 10 get_order /__classcall__ doctests still failing...
@willieab
Copy link
Author

@tscrim @dwbump

@willieab
Copy link
Author

willieab commented Apr 14, 2023

@dimpase @mkoeppe Could you please add me to the Sage triage team? I've contributed to Sage before, mostly to the fusion_rings folder: #30423 #31284 #30257 #29615, amongst others. @tscrim said I should contact you. Many thanks in advance!

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2023

I've sent the invite to org and team

@willieab
Copy link
Author

@mkoeppe Thanks!

@dwbump
Copy link
Contributor

dwbump commented Apr 14, 2023

@mkoeppe wrote:

I've sent the invite to org and team

I still can't add Willie as a reviewer on #35387

@tscrim
Copy link
Collaborator

tscrim commented Apr 18, 2023

Some quick comments on this for @willieab

  1. I pushed some changes to The Fusion Ring of the Drinfeld Double of a Finite Group #35387 that I would like to see pulled here and analogously made to the generic doc.
  2. We should do a deprecated lazy import of FusionRing in fusion_ring.py (for an example of this, see sage/rings/all.py).
  3. Don't make _repr_ an @abstract_method; it isn't (and shouldn't be) required.
  4. @abstract_method with a docstring doesn't need a pass (IMO, that actually makes it slightly more confusing).
  5. Remove _element_constructor; that is something for the internal's of Sage Parents.
  6. For VerlindeAlgebra, it is a bit more to code, but it seems natural to me to have it take a ``WeylCharacterRingring as input and have aclasscall_private` that parses everything to a WCR object to pass along to the `init` (with the appropriate additional options).

@dwbump
Copy link
Contributor

dwbump commented Apr 18, 2023

Also the tests show a problem with set_order. I think the test failures in verlinde_algebra.py need to be debugged.

@willieab
Copy link
Author

Thanks @dwbump @tscrim! I'll work on these changes. @tscrim Why do we need the deprecated lazy_import? We are neither changing the location nor the functionality of FusionRing.

@dwbump
Copy link
Contributor

dwbump commented Apr 24, 2023

Thanks @dwbump @tscrim! I'll work on these changes. @tscrim Why do we need the deprecated lazy_import? We are neither changing the location nor the functionality of FusionRing.

I started a PR that would import Travis' changes to #35387 and can probably finish it today or early tomorrow. I think doctest errors need investigating.

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2023

Why do we need the deprecated lazy_import? We are neither changing the location nor the functionality of FusionRing.

@willieab Because there is no longer a file fusion_ring.py. So if someone was directly importing from that file (say, in their own .py files/library), then their code will break. So it needs a lazy import redirect to the new location. (This might also be needed for old pickles too, but that is less important I think.)

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2023

Also, another general docstring comment. Remember that docstrings should have a short one-sentence description first, then followed by the more detailed paragraph explanation (if necessary).

dwbump pushed a commit to dwbump/sage that referenced this pull request Apr 25, 2023
@dwbump dwbump mentioned this pull request Apr 25, 2023
3 tasks
@dwbump
Copy link
Contributor

dwbump commented Apr 25, 2023

The PR #35569 brings in some of the edits to #35387, namely those to the main docstring in fusion_double.py.

@tscrim
Copy link
Collaborator

tscrim commented Apr 25, 2023

@dwbump It would be better for reviewer changes to have PRs done to the author’s individual fork rather than in the general Sage repo. Otherwise this creates separate threads/PRs that need further manual intervention and pollutes the search feature here.

@dwbump
Copy link
Contributor

dwbump commented Apr 26, 2023

@dwbump It would be better for reviewer changes to have PRs done to the author’s individual fork rather than in the general Sage repo. Otherwise this creates separate threads/PRs that need further manual intervention and pollutes the search feature here.

The only github repository I can push to is my own. Willie` can merge this PR into his. Eventually we can try to merge Willie's branch into sage. at which point there will be a (minor) problem, because the two branches are forks and the merge will not be a clean one. However that won't be too hard to take care of and I don't see a better way to proceed.

Am I missing something?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 26, 2023

From your repository you can open a PR against any fork of the Sage repo, including Willie's, not just the main repo.

@tscrim
Copy link
Collaborator

tscrim commented Apr 26, 2023

@dwbump It is as @mkoeppe said. We did this before during our Zoom call (and it is what I did with your branch). It also makes it easier for @willieab to merge it into his branch (and mark it as having done so).

@mkoeppe mkoeppe removed this from the sage-10.0 milestone May 4, 2023
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.

5 participants