Skip to content

feat: HasX attributes #34

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

nstarman
Copy link
Collaborator

@nstarman nstarman commented Jul 1, 2025

Requires #32. I'll rebase when that's in.

Now preceding #32.

@nstarman
Copy link
Collaborator Author

nstarman commented Jul 1, 2025

@jorenham @NeilGirdhar @lucascolley this doesn't resolve the current discussion re a DTypeT typevar or a DType protocol, but does mean Arrays can now understand any typevar or protocol we want to add.

Copy link
Member

@jorenham jorenham left a comment

Choose a reason for hiding this comment

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

I like the Has* protocols. Optype will cover the CanArray* ones I think. There might be some that can't be expressed because Self can't be passed as generic type argument, but I suppose we can deal with it when needed.

One nit is that ... are not needed when there's a docstring, which already counts as an expression or statement or something.

@nstarman
Copy link
Collaborator Author

nstarman commented Jul 2, 2025

I like the Has* protocols. Optype will cover the CanArray* ones I think. There might be some that can't be expressed because Self can't be passed as generic type argument, but I suppose we can deal with it when needed.

What do we do about docstrings?

One nit is that ... are not needed when there's a docstring, which already counts as an expression or statement or something.

Yes, my pylint is complaining. But IMO empty methods should have ... to distinguish them from ones that aren't empty. Helps in understanding inheritance with a Protocol.

@jorenham
Copy link
Member

jorenham commented Jul 2, 2025

What do we do about docstrings?

Hmm good question. Maybe a numpy-esque add_docstring function?

@jorenham
Copy link
Member

jorenham commented Jul 2, 2025

Yes, my pylint is complaining. But IMO empty methods should have ... to distinguish them from ones that aren't empty. Helps in understanding inheritance with a Protocol.

Yea I guess there's something to be said for that. It just looks a bit weird to me to see a ... occupy a line on its own (but that might have something to do with me spending most of my breathing time looking at stubs).

@nstarman nstarman force-pushed the has_x_attributes branch 2 times, most recently from 8096aff to 3793c1f Compare July 21, 2025 15:56
@nstarman nstarman force-pushed the has_x_attributes branch 5 times, most recently from cb3dbb3 to dec1842 Compare July 23, 2025 21:05
@nstarman nstarman marked this pull request as ready for review July 23, 2025 21:10
@nstarman nstarman requested a review from jorenham July 23, 2025 21:10
@nstarman
Copy link
Collaborator Author

@jorenham I added tests and made the protocols public.

@nstarman
Copy link
Collaborator Author

@jorenham This should cover all the array attributes.

@nstarman
Copy link
Collaborator Author

nstarman commented Aug 1, 2025

@jorenham it might be easier to merge this before doing numpy type compat stuff from #32.

@nstarman nstarman added this to the v2021-12-0.0 milestone Aug 1, 2025
@nstarman nstarman added the ✨ feature Introduce new features. label Aug 1, 2025
@nstarman nstarman added the ✅ tests Add, update, or pass tests. label Aug 1, 2025
"""Protocol for array classes that have a shape attribute."""

@property
def shape(self) -> tuple[int | None, ...]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in a followup we should add a type parameter UnknownDimsT = TypeVar(None, Never, default=None) so that it's possible to define an array where the dimensions are always known.

Array[..., UnknownDimsT=Never]

Then we can define the alias types

KnownDimsArray: TypeAlias = Array[..., Never]
UnknownDimsArray: TypeAlias = Array[..., None]

Copy link
Member

Choose a reason for hiding this comment

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

what would UnknownDimsT be the type of, then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

?

Copy link
Member

Choose a reason for hiding this comment

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

we should add a type parameter UnknownDimsT = TypeVar(None, Never, default=None)

as in; what will it annotate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dim types, like

def shape(self) -> tuple[int | UnknownDimsT, ...]:

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok, that makes sense. I was confused because it's a plural Dims.

...


class HasSize(Protocol):
Copy link
Member

Choose a reason for hiding this comment

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

What's the use-case of this one? Is there anything this can help with, that HasShape can't?
Put differently; should we make this public API or not?

...


class HasTranspose(Protocol):
Copy link
Member

Choose a reason for hiding this comment

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

Ah nice; this could be used to annotate functions that exclusively accept 2d arrays, without having to deal with shape-typing :)

HasNDim,
HasShape,
HasSize,
HasTranspose,
Copy link
Member

Choose a reason for hiding this comment

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

This HasTranspose restricts this to 2d arrays

Comment on lines +61 to +68
_: np.dtype[F32] = x_f32.dtype
_: np.dtype[I32] = x_i32.dtype
_: np.dtype[B] = x_b.dtype

# Check Attribute `.device`
_: object = x_f32.device
_: object = x_i32.device
_: object = x_b.device
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 go with typing.assert_type here

Comment on lines +76 to +88
_: int = x_f32.ndim
_: int = x_i32.ndim
_: int = x_b.ndim

# Check Attribute `.shape`
_: tuple[int | None, ...] = x_f32.shape
_: tuple[int | None, ...] = x_i32.shape
_: tuple[int | None, ...] = x_b.shape

# Check Attribute `.size`
_: int | None = x_f32.size
_: int | None = x_i32.size
_: int | None = x_b.size
Copy link
Member

Choose a reason for hiding this comment

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

typing.assert_type

...


class HasMatrixTranspose(Protocol):
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 move this one closer to HasTranspose

Copy link
Member

Choose a reason for hiding this comment

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

And the use-case of this is to require at least 2 dimensions, right?

...


class HasNDim(Protocol):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a situation where you'd wanna use HasNDim over e.g. HasShape? Otherwise we probably should keep this private, given that

There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.

"""Protocol for array classes that have a device attribute."""

@property
def device(self) -> object: # TODO: more specific type
Copy link
Member

Choose a reason for hiding this comment

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

I can see how this can be helpful for users if it would be generic. So I propose we either keep it private until that time, or to just make it generic right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature Introduce new features. ✅ tests Add, update, or pass tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants