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

dtypes and array scalars #13

Closed
wants to merge 3 commits into from
Closed

dtypes and array scalars #13

wants to merge 3 commits into from

Conversation

ev-br
Copy link
Collaborator

@ev-br ev-br commented Jan 3, 2023

identical to gh-12, only sent to the reductions branch, as per #12 (comment)

ev-br added 3 commits January 3, 2023 14:19
Try to duck-type NumPy, only without array scalars, so that

* dtype('float32').type --> np.float32

and

* np.float32(3) --> array(3.0, dtype='float32)

IOW, the attempt is to only have zero-dim arrays (we have them anyway)
and have them everywhere where NumPy creates a scalar.
@ev-br ev-br mentioned this pull request Jan 3, 2023
@asmeurer
Copy link
Member

asmeurer commented Jan 3, 2023

I guess there's a lot of other methods on numpy dtypes that people might care about (although none of them are guaranteed by the array API FWIW). There's also the extensive __eq__ that is defined on numpy dtypes (see also data-apis/array-api#582). NumPy also has the distinction between dtype classes and instances (e.g., np.float64 vs. np.dtype('float64'), which I personally find annoying and I don't know if it needs to exist here.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This looks like a good approach overall. I think it will work well enough. It would be useful to add the main properties that are supported in a docstring (probably generic, because that's where the functionality is).

There's also the extensive __eq__ that is defined on numpy dtypes (see also data-apis/array-api#582).

__eq__ would indeed be good to support as much as possible.

NumPy also has the distinction between dtype classes and instances (e.g., np.float64 vs. np.dtype('float64'), which I personally find annoying and I don't know if it needs to exist here.

Agreed it's annoying, but it's probably easier to keep that here. The correspondence in this PR isn't perfect (see example below), but close enough.

>>> type(tnp.dtype('float64'))
<class 'torch_np._dtypes.dtype'>
>>> type(np.dtype('float64'))
<class 'numpy.dtype[float64]'>

arg1 = dtype(arg1).type
if not issubclass_(arg2, _scalar_types.generic):
arg2 = dtype(arg2).type
return issubclass(arg1, arg2)
Copy link
Member

Choose a reason for hiding this comment

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

This seems to not yet be complete I think? At least from a quick test:

In [25]: tnp.issubdtype(tnp.float32, tnp.inexact)
Out[25]: False

In [26]: np.issubdtype(np.float32, np.inexact)
Out[26]: True

@@ -183,6 +181,23 @@ def is_integer(dtyp):
return dtyp.typecode in typecodes['AllInteger']



def issubclass_(arg, klass):
Copy link
Member

Choose a reason for hiding this comment

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

This goes on my list of functions to deprecate and remove in NumPy 2.0 ...

Copy link
Member

Choose a reason for hiding this comment

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

This one is easy enough so may as well leave it here now. I'd say let's not bother with issubsctype, obj2sctype, etc., those definitely all need to go from NumPy.

An error occurred while trying to automatically change base from reductions to main January 4, 2023 10:06
An error occurred while trying to automatically change base from reductions to main January 4, 2023 10:06
An error occurred while trying to automatically change base from reductions to main January 4, 2023 10:07
@ev-br ev-br deleted the branch reductions January 4, 2023 10:08
@ev-br ev-br closed this Jan 4, 2023
@ev-br
Copy link
Collaborator Author

ev-br commented Jan 4, 2023

Apparently merging a PR and deleting the PR branch closes the PR to this branch. Which is not unreasonable, now that I think about it. Let's continue in gh-12, which is the same PR against main.

@ev-br ev-br mentioned this pull request Jan 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