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.
ENH: Add cumulative methods to ea #48111
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
ENH: Add cumulative methods to ea #48111
Changes from all commits
2897723
a54e1b4
10abc0f
c2d7592
2c149c0
79cea11
12a5ca3
dc959f4
bcfb8a8
9a8f4ec
5d837d9
9e9f0c3
a1a1cb2
6d967ad
73363bf
0d9a3d5
84a7d81
ce6869d
3b5d1d8
0337cb0
f0722f5
99baa1b
fa35b14
43fca7c
7bd6378
185510b
2ef9ebb
7d898bd
09b42be
af0dd24
bc9a36a
38454a3
5ecfa51
8d62594
5f3b624
06d1286
7efcb5f
ae5f969
f7e3f4f
aa99927
9cab6d9
b3ae864
52e6486
99fb664
d339250
be6f974
a902f4e
64afb5b
1e5d77b
c95b490
dc669de
08475a4
67fa99a
a36632b
b3d3c81
f8f6367
ad6773d
663c301
e17f3a0
8cb66f9
4f953cf
b33a5df
18ec178
305bdc7
56bfb23
63db854
9c91c55
6ba3ca9
f2a49b3
386fa39
55de384
6a5b7f8
9c63c64
84dd141
2f23499
a5b30e6
d22c8a0
a5866c7
8255457
483b608
150fd3b
80e2dc6
dceab99
1c14f18
e20501a
53147c4
597e978
5ebe8ea
32367c0
1fc7a39
628611c
d884845
054ad94
64219d9
a8645db
597dd84
13b2633
2acc7a8
a410a88
a066588
580267b
54aa8a8
fecdc42
a8eb63a
19f36cf
67c6327
fb4fdc2
9c81a1d
6e2b453
cdca590
5c9a940
6765fe1
d3be9f3
b4eb0fd
611b85e
a6a974a
e7364bd
4ff6e4d
57abcc3
1b3771e
1267961
c770872
cb7277b
797e724
4f8b06a
ab3cf7e
e1d2a4e
53eac54
e7dbd5f
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 the numpy functions not work directly?
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.
This has a different behavior than going through nanops. The initial pr included this but I ripped it out to keep it a bit more focused. Plan to tackle this afterwards, but we have to decide what we actually want here first. Hence only doing masked ops 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.
can you expand on this?
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.
skipna is the problem, difference in behavior with regards to floats, remembered this incorrect.
See #28509 (comment)
Wanted to investigate this as a follow up when we can check this more isolated
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.
this looks like it might choke on PeriodDtype?
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 to work, could you elaborate what you suspect?
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 think this goes wrong when there's a NaT present
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.
This would restore the previous behavior (but the previous behavior was wrong as well...). Are you ok with addressing this in a follow up?
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.
previous as of when? IIRC this was last changed multiple years ago.
so does this PR get the behavior right for dt64/td64? If so, the solution for PeriodDtype is similar to median/min/max to do a view to dt64, do the op, then view back.
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.
Sorry, my explanation was a bit confusing.
The initial pr which this work is based on tried the following:
This got confusing, since it was pretty big and the new date time logic changed the behavior. So I made the decision to reduce scope and only tackle the first 2 points and defer the third to a follow up.
To achieve this, I just send the date time logic through nanops again (this is what I meant with previous behavior, previous as in before _accumulate was added to the ea interface). This avoid any change in behavior for the date time was and should keep review more focused.
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.
Let's try this one more time, thanks for your patience in explaining this to me.
IIUC this PR should not change any behavior for dt64/dt64tz/td64 dtypes, correct?
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, correct. Want to do those changes as follow ups
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.
Cool, ill trust you to handle these.
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.
Thx, yep want to get this into 2.0 as well