Skip to content

QuaternionAlgebra: Unify ABC and factory#38228

Open
mkoeppe wants to merge 3 commits intosagemath:developfrom
mkoeppe:QuaternionAlgebra_abc
Open

QuaternionAlgebra: Unify ABC and factory#38228
mkoeppe wants to merge 3 commits intosagemath:developfrom
mkoeppe:QuaternionAlgebra_abc

Conversation

@mkoeppe
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe commented Jun 16, 2024 •
edited
Loading

We make a change:

  • from a UniqueFactory QuaternionAlgebra and abstract base class QuaternionAlgebra_abstract
  • to an abstract base class QuaternionAlgebra with a __classcall_private__ method

What is done here: We consider QuaternionAlgebraFactory internal and remove it outright. (+ docstring/doctest cosmetics)

Possible implementation variants:

  • Minimally invasive change: Keep the UniqueFactory and delegate to it by __classcall_private__. (This was done in c2d2fca)
  • Change that maintains full compatibility: Add class methods QuaternionAlgebra.create_key, QuaternionAlgebra.create_object that delegate to the factory functions, so that calls to create_key, create_object continue to work without change
  • This could be combined with deprecating QuaternionAlgebraFactory.

Also @saraedum (who introduced the use of UniqueFactory for this class)

  • Does UniqueFactory offer any benefits over CachedRepresentation/UniqueRepresentation at all, or should we just replace uses of UniqueFactory by the more commonly used CachedRepresentation/UniqueRepresentation?

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 16, 2024 •
edited
Loading

Documentation preview for this PR (built with commit ed82345; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tscrim
Copy link
Copy Markdown
Collaborator

tscrim commented Jun 19, 2024

The factory is doing nothing special. +1 to converting it to the ABC, but it should also inherit from UniqueRepresentation (or at least CachedRepresentation, but I don't see a need for it to have its own equality) to match the previous behavior.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 19, 2024

@tscrim Do you have a specific opinion on the three "open questions" in the PR description?

@tscrim
Copy link
Copy Markdown
Collaborator

tscrim commented Jun 19, 2024

I am for deprecating it. Although I can be convinced that could be removed outright (with an understanding that it is breaking our deprecation policy, but I think it can be justified in doing so here).

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 19, 2024

OK, then let's remove it outright. Thanks

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 23, 2024

The failure in CI Conda is unrelated.

@tscrim
Copy link
Copy Markdown
Collaborator

tscrim commented Jul 1, 2024

Would it be possible to have QuaternionAlgebra_abstract as a lazy_import of QuaternionAlgebra with a deprecation?

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jul 1, 2024

As discussed in #38207 (comment), importing such a lazy_import will work when this class is used in isinstance. But it won't work it the user tries to subclass that class.

@tscrim
Copy link
Copy Markdown
Collaborator

tscrim commented Jul 1, 2024

However it will still emit a message if used, even if the code breaks on subclassing. That is better than it suddenly disappearing when the deprecation period is over.

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

Reviewers

@tscrim tscrim Awaiting requested review from tscrim

Assignees

@mkoeppe mkoeppe

Labels

c: algebra s: needs review t: refactoring v: moderate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mkoeppe @tscrim