-
-
Notifications
You must be signed in to change notification settings - Fork 19k
BUG: Dataframe arithmatic operators don't work with Series using fill_value #61828
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
Open
eicchen
wants to merge
19
commits into
pandas-dev:main
Choose a base branch
from
eicchen:BUG-#61581-DataFrame.mul
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
a9c8d85
Initial test case
eicchen f303a04
Updated test case to account for results of mul being NaN if both inp…
eicchen 5ac26a4
Removed test cases which expect an error from fill_value
eicchen a60fbb0
Updated test case to include other operators which included fill_value
eicchen 87ecfc4
Removed restriction on using fill_value with series
eicchen bc805fd
Included PR suggestions, added seperate dtype test (WIP)
eicchen be09616
temp files
eicchen 1ebcf6e
Added test case to test EA and NUMPY dtypes
eicchen 98fb07f
Merge branch 'pandas-dev:main' into BUG-#61581-DataFrame.mul
eicchen a5940d5
addressed changes brought up in PR, converted test cases to not use n…
eicchen dcf3391
Limit np conversion to IntegerArray and FloatArray
eicchen 1179098
Updated EA catch method in _maybe_align_series_as_frame
eicchen 2dfb4bf
Addressed errors from changes in som tests
eicchen ce0b2ef
removed comment and errant print statement
eicchen eaac655
Commented out test_add_frame's xfail to test CI
eicchen a719a1d
Allows frames to be added to strings, with modifications to tests tha…
eicchen 06bcebc
Merge branch 'main' into BUG-#61581-DataFrame.mul
eicchen 801b39e
Moved type conversion within add and radd if statement, removed datea…
eicchen 4be6817
Removed PeriodArray special casing and modified test case
eicchen 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
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
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
Oops, something went wrong.
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.
is it obvious this is always right? e.g. what if self is
pa.timestamp("us")
and other ispa.int64()
?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 fair, I did try to only check for NullArrays, but that returned the error about how it couldn't concatenate the frame in the original add_to_frame testcase.
We could circumvent that by casting the initial df as an object but I didn't want to mess with the test case because I didn't know if that was something it was testing for.
Alternatively, I can just reimplement a check and check for dtypes we'd want to let go through
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.
looks like this is the cause of a bunch of test failures
FAILED pandas/tests/extension/test_arrow.py::test_arithmetic_temporal[pa_type11] - pyarrow.lib.ArrowNotImplementedError: Unsupported cast from duration[us] to timestamp using function cast_timestamp
.are you running the tests locally before committing/pushing?
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 only ran the array folder because the full suite takes a lot of time, Ill be sure to run the full thing going forward. That's on me.
Uh oh!
There was an error while loading. Please reload this page.
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.
Ill add official testcases once the build clears CI due to the weird tack-on nature of this bug fix. Just from some local testing, it looks like there is already a preexisting error message for trying to use the add operation on dtypes like Datetime and TimeDelta.
That being said, it looks like the CI is throwing errors on some of the builds but not others again, and what do you know, they're not replicated on my local machine. Would you know who I could talk to to figure out why that is?
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 the exception message in a test needs to be updated thats fine as long as the new one makes sense.
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.
Sounds good to me.
Any pointers for the CI or should I ask it during the meeting tmr?
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 havent looked too closely, but the CI failiures ll look like cases of "the test needs to be updated to check for the new exception message".
none of the edits to the ArrowEA are necessary, nor is the special-casing for Period.
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.
The issue is that 2/3 of the unit tests succeed as-is, so it doesn't make sense why only 7 are failing. Especially since the error is about a float being concatenated with a string, which all the other builds are able to do. My guess was that something was different about their set up process.