Skip to content

Add an is_dtype function #199

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

Merged
merged 9 commits into from
Aug 2, 2023
Merged

Add an is_dtype function #199

merged 9 commits into from
Aug 2, 2023

Conversation

MarcoGorelli
Copy link
Contributor

No description provided.

----------
dtype: Any
The input dtype.
kind: str
Copy link
Contributor Author

@MarcoGorelli MarcoGorelli Jul 12, 2023

Choose a reason for hiding this comment

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

the array api also allows dtype, but I don't think we need that here. If you want to check whether a dtype is of a specific dtype, you can just use isinstance, e.g.

isinstance(column.dtype, namespace.Float64)

I think this function is more useful for checking conceptual hierarchies, like 'integral'

So, for now, I'm only allowing str | tuple[str, ...]

Copy link
Contributor

Choose a reason for hiding this comment

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

Part of the reason for allowing dtype instances is to allow arbitrary composition of dtype sets beyond the supported kinds and allow an escape hatch beyond the kinds officially supported by the array API standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't you do that with

is_dtype(column.dtype, 'integral') or isinstance(column.dtype, MyFancyDType)

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but that becomes a bit unwieldy quickly if you want to arbitrarily combine multiple "kinds" and/or individual dtypes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest keeping this simple to begin with, we can always increase complexity later if necessary

Copy link
Member

Choose a reason for hiding this comment

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

I'd have made the same comment as @kgryte, I think it'll be needed and is better than using isinstance. However, it's a fully backwards-compatible extension, so I'm fine with leaving this comment unresolved here and a follow-up PR to extend the functionality to:

kind: str | tuple[str, ...] | DType | tuple[DType, ..]

@MarcoGorelli MarcoGorelli requested a review from rgommers July 26, 2023 10:07
@MarcoGorelli MarcoGorelli requested a review from kkraus14 August 1, 2023 13:19
@rgommers rgommers changed the title Add is_type function Add an is_dtype function Aug 2, 2023
@rgommers
Copy link
Member

rgommers commented Aug 2, 2023

Somehow gh doesn't let me merge main again (to unbreak CI), can you rebase or merge main again Marco?

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.

LGTM to merge - it's not complete, as discussed in the open comment thread, but this works as is and the rest can be done later.

Thanks @MarcoGorelli and @kgryte

----------
dtype: Any
The input dtype.
kind: str
Copy link
Member

Choose a reason for hiding this comment

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

I'd have made the same comment as @kgryte, I think it'll be needed and is better than using isinstance. However, it's a fully backwards-compatible extension, so I'm fine with leaving this comment unresolved here and a follow-up PR to extend the functionality to:

kind: str | tuple[str, ...] | DType | tuple[DType, ..]

@rgommers rgommers merged commit b03bb4f into data-apis:main Aug 2, 2023
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.

3 participants