Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API: added array #23581
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
API: added array #23581
Changes from 43 commits
bfefc96
51480a3
dcb7931
a635649
fb0d8bc
d58a320
fe06de4
72f7f06
c02e183
a2d3146
37901b0
4403010
9401dd3
838ce5e
5260b99
248e9e0
cf07c80
22490a8
5e0dc62
fe40189
7eb9d08
fa7b200
1ca14fe
4473899
382f57d
dd76a2b
159d3a2
5366950
c818a8f
ba8b807
77cd782
dfada7b
5eff701
9406400
8eb07c3
ea3a118
ecae340
fb814fc
a6f6d29
6c243f3
86b81b5
2c6cf97
50d4206
9e1b4e6
000967d
bf829c3
faf114d
3186ded
1c4da0e
36c6f00
932e119
45d07eb
d1aba73
981f735
1f3bb50
c8d3960
1b9e251
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specify this as 1D here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this statement a bit misleading, as we actually don't infer from scalars (at least not in the sense of eg how Series does it), we only use numpy's inference?
If we say that we infer, I would expect those to do the same:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looking now further down in the implementation :-) I see you do handle this case for Period, so here it indeed infers:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't stated it explicitly, but it would be nice if 3rd parties could eventually hook into this as well. Right now I think it's just Period (and maybe interval?) that get inferred. Maybe timestamps with timezones once DatetimeArray is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And not Timestamps without timezones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that
lib.infer_dtype
doesn't distinguish the twoI've added interval to what we'll infer. Perhaps we should be explicit in the docs for that? Though that kinda closes the door to inference for 3rd party arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would do that.
Would we, in the longer term, the inference happening here be the same as the inference happening in
Series
? Or would that actually add to much corner cases from there we don't want to carry over?How would you envision third party arrays participate in inference? That seems a bit difficult in any case (trying out all registered ones, ..?), and IMO more error prone for users (if you forget to import the 3rd party library, you silently get different results)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I want (long term).
Haven't thought about it beyond "check for 3rd party scalar types". I'm not familiar with how infer_dtype works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add an example which raises for scalars, 2d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomAugspurger do you remember if there was any particular reason for using this pattern instead of
dtype = pandas_dtype(dtype)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall. I wonder if this predates
pandas_dtype
handling extension dtypes.