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

Sync some changes from KvikIO's Array #1080

Open
wants to merge 3 commits into
base: branch-0.41
Choose a base branch
from

Conversation

jakirkham
Copy link
Member

Pulls in a few changes from KvikIO's Array. Namely...

  • asarray for easy conversion to Array (no-op if already an Array)
  • Include types for a few more properties

@jakirkham jakirkham requested a review from a team as a code owner September 28, 2024 01:11
@jakirkham jakirkham added enhancement New feature or request non-breaking improvement and removed enhancement New feature or request labels Sep 28, 2024

T = TypeVar("T")

class Array(Generic[T]):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This a slightly weird use of a generic type. Usually one would read a type Array[T] as "an array containing objects of type T", that is for x : Array[T], then x[i] : T forall i \in [0, len(x)). However, here you're using it to mean the wrapped type.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is taken from verbatim from KvikIO. Appears this was added in PR ( rapidsai/kvikio#214 ), which it looks like you approved 😉

That said, no objection to changing it to something else. Could you please submit a PR to KvikIO with the syntax you prefer? Can then incorporate it here 🙂

ucp/_libs/arr.pyx Outdated Show resolved Hide resolved
Co-authored-by: Peter Andreas Entschev <[email protected]>
Copy link
Member

@pentschev pentschev left a comment

Choose a reason for hiding this comment

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

LGTM in spite of @wence-'s comment, that can probably be addressed in KvikIO first and then again incorporated here but it's probably a good idea to let Lawrence voice his (dis-)approval of that too. Thanks @jakirkham .

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