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.
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
implement+test mean for datetimelike EA/Index/Series #24757
implement+test mean for datetimelike EA/Index/Series #24757
Changes from 7 commits
434e2cd
c8abf33
30eeb64
d48e2ef
0e32be2
231458d
1129e8c
38f829a
aba90ec
176b355
ccb790b
4f4cb6d
ec83db1
5fb1db9
028c789
50e714e
1b24e7d
4d40906
a49da37
94d3466
e4e6a03
7953a7b
15307da
da719e1
637b415
58bca36
abcb87a
4df0b1c
c9736d7
d2f5e6f
de9025c
4c553a9
330fc41
7c6201b
642d4e2
7b9fd42
4be012f
5682b65
581ff1a
34a83a4
14150ee
450c8ce
3e31ca1
111c345
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.
Do we want to add an
axis
argument here?I know we have already added in some places, but also not added in other places, so probably something we should discuss.
I would personally try to not add too much overhead in additional useless kwargs just for matching signatures, if they can be avoided.
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.
IIRC, this lets us hook into
np.mean
, but we should verify exactly which kwargs are necessary.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, that's why we did it in the past (but for that we also need the additional kwargs). But, for EAs, we could also decide to implement the array function protocol to achieve this goal, which might obviate the need to add the additional kwargs
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.
@jbrockmendel i think its worthile here to allow calling
np.mean
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.
don'e we also need out=? meaning we should just accept kwargs like we do for Series.mean
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.
If it is to match numpy's signature, then there are a bunch of other kwargs to add as well
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.
right and we do this elsewhere
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.
would just add kwargs for this purpose; we don't add them explicity anywhere else
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.
seems very duplicative of the test below, is there a reason?
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.
bc at the time when this was written Index reduction tests had not been collected in tests.reductions. Will move/de-duplicate.