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

Fix #105 #106

Merged
merged 1 commit into from
Sep 20, 2023
Merged

Fix #105 #106

merged 1 commit into from
Sep 20, 2023

Conversation

PhilipVinc
Copy link
Collaborator

We were providing the wrong class to the __init_subclass__called in our initialisation of parametric subclasses. This fixes it and adds a test.

@PhilipVinc
Copy link
Collaborator Author

@femtomc CI is broken by the latest release of bear type, but this should work. Can you test it on your code and let me know if it solves it even in the more realistic/complex scenario?

@femtomc
Copy link

femtomc commented Sep 18, 2023

Will do, one moment.

@femtomc
Copy link

femtomc commented Sep 18, 2023

@PhilipVinc That fix works to get past the first error! -- but I'm actually finding now that some of my methods (defined via dispatch inside of classes) are not correctly dispatching based on covariance.

E.g. in a base class (call it Model) which I mix in, I define a method with signature:

    @dispatch
    def update(
        self,
        key: PRNGKey,
        prev: Trace,
        new_constraints: Mask[ChoiceMap],
        argdiffs: Tuple,
    ) -> Tuple[Any, FloatArray, Trace, Mask[ChoiceMap]]:

Now, in a subclass which mixes in Model, I try and invoke update with a Mask[T1], where T1 <: ChoiceMap.

However, this fails on the resolver:

plum.resolver.NotFoundLookupError: For function `update` of `Model`, `(Normal(), Array([4171552086, 1934896579], dtype=uint32), DistributionTrace(gen_fn=Normal(), args=(0.0, 1.0), value=Array(-0.10823099, dtype=float32), score=Array(-0.9247955, dtype=float32)), <Mask[ValueChoiceMap] object at 0x7fac1c6527d0>, (Diff(primal=0.0, tangent=_NoChange()), Diff(primal=1.0, tangent=_NoChange())))` could not be resolved.

I can pull this into a separate issue.

Edit: just to be clear, it also fails on the dispatch definition with this signature:

@dispatch
def update(
    self,
    key: Any,
    prev: Any,
    new_constraints: Mask[ChoiceMap],
    argdiffs: Any,
) -> Any:

e.g. totally permissive except for my parametric class

@PhilipVinc
Copy link
Collaborator Author

If you can provide a MWE that does not work I can give a look.
I suspect some bugs might be in the underlying bear type, which might not correctly treat type parameters correctly.

@wesselb
Copy link
Member

wesselb commented Sep 20, 2023

There is one failing test, but that failure seems unrelated to this, so I'm happy to merge this if you are too. :)

@PhilipVinc and @femtomc, I believe this might be the MWE that you are looking for:

from numbers import Number
from plum import parametric


@parametric
class A:
    def __init__(self, x):
        self.x = x

Then

>>> issubclass(A[int], A[int])  # Just a check
True

>>> issubclass(A[int], A[Number])  # Covariance!
True

>>> isinstance(A[int](None), A[Number])  # Here is where it goes wrong :((( This breaks dispatch.
False

I've been vaguely aware of this for a while, but never really took the time to dig into it. The problem is that we have not implemented CovariantMeta.__instancecheck__. I'm not 100% sure what exactly the right way to implement CovariantMeta.__instancecheck__ is. My sense is that the the default __instancecheck__ works fine, except when the other argument is another instance of the parametric class. Hence, once option would be to handle this special case.

Here's how that would work in the above example:

from numbers import Number
from plum import parametric


class AMeta(type):
    def __instancecheck__(self, x):
        return issubclass(type(x), self)

@parametric
class A(metaclass=AMeta):
    def __init__(self, x):
        self.x = x

Then

>>> issubclass(A[int], A[int])
True

>>> issubclass(A[int], A[Number])
True

>>> isinstance(A[int](None), A[Number]) 
True

so we're all good.

I have two concerns about this:

  1. Is isinstance(type(x), self) truly correct here? Can this give False in situations where it should give True? (This is related to our made-up notion of faithfulness.) Is there a way to implement this without appealing to type(x)?

  2. Are there any performance implications for reducing instance checks to subclass checks? parametric is already pretty slow, so maybe this is not a problem.

@PhilipVinc
Copy link
Collaborator Author

yes the failure is unrelated and due to the resolve pep bug in beartype. I'll merge this fix and we can experiment with fixing the other issue raised in a separate PR.

@PhilipVinc PhilipVinc merged commit 9ceec86 into beartype:master Sep 20, 2023
2 of 6 checks passed
@PhilipVinc PhilipVinc deleted the pv/fix-parametric branch September 20, 2023 21:45
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