-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: incorrect EA casting in groubpy.agg #38254
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
Closed
jbrockmendel
wants to merge
23
commits into
pandas-dev:master
from
jbrockmendel:ref-python_agg_general
Closed
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
99073b8
REF: consolidate casting
jbrockmendel 1ed9d8a
lighter-weight casting
jbrockmendel 21b4f0f
simplify casting
jbrockmendel 56b42bb
REF: minimize python_agg_general groupby casting
jbrockmendel e01487f
Handle Float64
jbrockmendel cf5bd53
TST: PeriodDtype
jbrockmendel 02bffdb
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel f950c75
dont cast
jbrockmendel 0544c8b
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 5a30b93
whatsnew
jbrockmendel 3410102
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel b481049
move whatsnew to 1.3.0
jbrockmendel 2047f3c
CLN: remove unused import
jbrockmendel b96835f
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 7bc78eb
retain Float64
jbrockmendel 643fab6
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel eddb089
fix test
jbrockmendel 2572dda
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 5590ad7
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 5b73850
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 896a97a
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 5e611b2
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel 961304f
Merge branch 'master' of https://github.com/pandas-dev/pandas into re…
jbrockmendel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Inside
maybe_cast_result
, we already have a special case check for not casting back when having datetime or categorical data. You could add periods to that check, and then this line can be left as is, while still fixing the period issue.Then we can add the period test, without needing to change the decimal tests.
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 actually what I find most objectionable about maybe_cast_result. It is a kludge that special-cases pandas-internal EAs.
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.
But as I said below, it would at least be a consistent "kludge"... ;)
Not calling the method here that was exactly written to be called here, it not making it any less kludgy IMO.
So I think it would be good to add the period test case here, and then focus on #38315 to properly fix it.
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 is the only remaining usage of that kludge, so getting rid of it would certainly make it less kludgy.
It's going to be a while before 1.3, so I'm fine sticking a pin in this to see if a better solution is implemented between now and then. If it doesn't, we should do this for 1.3.