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

feat(DRAFT): DType.__eq__ as a TypeIs[...] #2050

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Feb 19, 2025

What type of PR is this? (check all applicable)

  • πŸ’Ύ Refactor
  • ✨ Feature
  • πŸ› Bug Fix
  • πŸ”§ Optimization
  • πŸ“ Documentation
  • βœ… Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below

Important

Seems that we can't represent this well in the current typing spec AFAICT.
See (dbadb09) message

https://typing.readthedocs.io/en/latest/spec/narrowing.html

Type checkers should assume that type narrowing should be applied to the expression that is passed as the first positional argument to a user-defined type guard.
If the type guard function accepts more than one argument, no type narrowing is applied to those additional argument expressions.

If a type guard function is implemented as an instance method or class method, the first positional argument maps to the second parameter (after β€œself” or β€œcls”).

So __eq__ currently describes the opposite of the spec:

import narwhals as nw

dtype = nw.Boolean()
second_parameter = nw.Int64

dtype == second_parameter
dtype.__eq__(second_parameter)

# Equivalent to
isinstance_or_issubclass(second_parameter, type(dtype))
isinstance_or_issubclass(nw.Int64, nw.Boolean)
issubclass(nw.Int64, nw.Boolean)

@dangotbanned dangotbanned added enhancement New feature or request typing labels Feb 19, 2025
@@ -215,7 +215,7 @@

# DTypes
dtypes = [
i for i in nw.dtypes.__dir__() if i[0].isupper() and not i.isupper() and i[0] != "_"
i for i in nw.dtypes.__all__ if i[0].isupper() and not i.isupper() and i[0] != "_"
Copy link
Member Author

Choose a reason for hiding this comment

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

FIXME

Need some help getting this work again
https://results.pre-commit.ci/run/github/760058710/1739990897.pkejAgUjQ3CdKYXvgIDqPw

I don't understand what the two checks are doing https://narwhals-dev.github.io/narwhals/api-reference/dtypes/#narwhals.dtypes

Or why this seems to include Literal?

BASE_DTYPES = {
"NumericType",
"DType",
"TemporalType",
"Literal",
"OrderedDict",
"Mapping",
}

`pyright` was yelling in `dtypes_test`
- I've played around with this for quite a while now
- I don't think there's a satisfying way to get `__eq__` working the same as `isinstance_or_issubclass`
- Generic `Datetime` & `Duration` are achievable
@dangotbanned
Copy link
Member Author

@MarcoGorelli I've added a little write-up for what I believe is the blocking issue.

Maybe this is a naive question, why was __eq__ chosen for this instead of __(instance|subclass)check__?
https://docs.python.org/3/reference/datamodel.html#customizing-instance-and-subclass-checks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked enhancement New feature or request typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant