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

make @parametric work with classes with arbitrary metaclasses #34

Closed
wants to merge 3 commits into from

Conversation

PhilipVinc
Copy link
Collaborator

I am experimenting with https://github.com/cgarciae/treeo and plum, and found out that since both inject meta classes they don't work together.

This PR makes plum work with classes which already have a meta class by injecting CovariantMeta as a superclass of the existing meta class, and by redefining __call__ (essentially the constructor) to infer the parameters before calling into the existing class.

I'm surprised this was so easy to implement!

@PhilipVinc PhilipVinc changed the title make @parametric work with classes with arbitrary metaclass make @parametric work with classes with arbitrary metaclasses Dec 4, 2021
@coveralls
Copy link

coveralls commented Dec 4, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling a0df7ac on PhilipVinc:pv/parametricmeta into b521bad on wesselb:master.

Without this bugfix, the __init_subclass__ was called on the original (non parametric) class that is never returned to the user.
@wesselb
Copy link
Member

wesselb commented Dec 9, 2021

Thank you, @PhilipVinc!! This really is excellent—a strict improvement. Merging this now and releasing a new version. :)

@wesselb
Copy link
Member

wesselb commented Dec 9, 2021

Actually, you've still marked this as a draft. The PR looks complete to me. Are you happy to have this merged?

@PhilipVinc
Copy link
Collaborator Author

Yeah, I think I wanted to add a test to check for what the fix issue in __init_subclass__ commit fixed.
I found out about it in a very exotic case that arises when you combine @parametric with classes that auto-register themselves with jax, but it's a real issue of plum,.

I'll add a test soon

@wesselb wesselb mentioned this pull request Mar 3, 2023
@wesselb
Copy link
Member

wesselb commented Mar 3, 2023

The contents of this PR will be included in #68, so closing this.

@wesselb wesselb closed this Mar 3, 2023
wesselb added a commit that referenced this pull request Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants