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

Convert self object manually to np.ndarray on fallback #430

Closed

Conversation

manopapad
Copy link
Contributor

Otherwise we get errors like the following when we try to invoke an unimplemented array-level method:

TypeError: descriptor 'std' for 'numpy.ndarray' objects doesn't apply to a 'ndarray' object

Otherwise we get errors like the following when we try to invoke an
unimplemented array-level method:

TypeError: descriptor 'std' for 'numpy.ndarray' objects doesn't apply to a 'ndarray' object
@manopapad manopapad requested a review from bryevdv June 28, 2022 15:31
@@ -150,18 +154,23 @@ def wrapper(*args: Any, **kwargs: Any) -> Any:
location=location,
implemented=False,
)
if is_method:
args = (args[0].__array__(),) + args[1:]
return func(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would isinstance check with types.MethodType work instead of having to pass an explicit flag?

@bryevdv
Copy link
Contributor

bryevdv commented Jun 28, 2022

@manopapad this change also seems to work, without needing to change API:

diff --git a/cunumeric/coverage.py b/cunumeric/coverage.py
index 2056bbed..07b3f72c 100644
--- a/cunumeric/coverage.py
+++ b/cunumeric/coverage.py
@@ -150,6 +150,8 @@ def unimplemented(
                 location=location,
                 implemented=False,
             )
+            if isinstance(func, (MethodType, MethodDescriptorType)):
+                args = (args[0].__array__(),) + args[1:]
             return func(*args, **kwargs)
 
     else:
@@ -162,6 +164,8 @@ def unimplemented(
                 stacklevel=stacklevel,
                 category=RuntimeWarning,
             )
+            if isinstance(func, (MethodType, MethodDescriptorType)):
+                args = (args[0].__array__(),) + args[1:]
             return func(*args, **kwargs)
 
     wrapper._cunumeric = CuWrapperMetadata(implemented=False)

Let me open a PR with this and also try to add a test for it.

@bryevdv
Copy link
Contributor

bryevdv commented Jun 28, 2022

Although, either of the changes means that clone_class is now only useful for ndarray, since it now implicitly assumes all self args should be arrays. If we want clone_class to retain any semblance of being a generic tool, we will have to find a way not to hard-code this codepath unconditionally for any and every method.

@bryevdv
Copy link
Contributor

bryevdv commented Jun 28, 2022

Ugh. Testing this is a mess. We can't just make some toy class in the test, because the numpy gets unhappy. But if we base tests on real ndarray then things that are unimplemented today, may be implemented in the future, negating the the test.

@manopapad
Copy link
Contributor Author

Superseded by #431

@manopapad manopapad closed this Jul 18, 2022
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.

2 participants