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

QuaternionAlgebra: Unify ABC and factory #38228

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Jun 16, 2024

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

Copy link

github-actions bot commented Jun 16, 2024

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

@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch 3 times, most recently from 6e8d71f to c2d2fca Compare June 16, 2024 22:23
@mkoeppe mkoeppe marked this pull request as ready for review June 16, 2024 22:57
@mkoeppe mkoeppe requested a review from tscrim June 19, 2024 01:54
@tscrim
Copy link
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
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
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
Contributor Author

mkoeppe commented Jun 19, 2024

OK, then let's remove it outright. Thanks

@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch from e328a83 to a74097b Compare June 22, 2024 23:10
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 23, 2024

The failure in CI Conda is unrelated.

@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch from a74097b to 5c7af31 Compare June 29, 2024 02:25
@tscrim
Copy link
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
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
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.

@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch from 5c7af31 to ed82345 Compare July 25, 2024 09:58
@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch from ed82345 to e2f933e Compare August 3, 2024 18:19
@mkoeppe mkoeppe force-pushed the QuaternionAlgebra_abc branch from e2f933e to adb137a Compare August 11, 2024 04:04
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.

2 participants