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

fix: nbytes repr for typetracers with nbytes=unknown_length #3361

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

pfackeldey
Copy link
Collaborator

This was an oversight from #3348

The nbytes property of a typetracer can be the unknown_length sentinel, which doesn't tell us anything about the number of bytes of the array. Thus, we can't add it to the repr.

@pfackeldey pfackeldey requested a review from ianna January 7, 2025 14:22
@agoose77
Copy link
Collaborator

agoose77 commented Jan 7, 2025

@pfackeldey given that we're allowing reprs to be "non-touching" (in the sense that we don't error if a repr happens after column optimisation, where there are placeholders), I think this can probably say "nbytes: unknown".

@pfackeldey
Copy link
Collaborator Author

@pfackeldey given that we're allowing reprs to be "non-touching" (in the sense that we don't error if a repr happens after column optimisation, where there are placeholders), I think this can probably say "nbytes: unknown".

That's a very good point! I'll update the PR accordingly 👍

Copy link
Collaborator

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@pfackeldey - Thanks for fixing it promptly! Looks good to me. Please, merge it if you finished with it. Thanks!

@pfackeldey pfackeldey merged commit 19ffb38 into main Jan 7, 2025
39 checks passed
@pfackeldey pfackeldey deleted the pfackeldey/fix_nbytes_repr_unknownlength branch January 7, 2025 19:19
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