Skip to content

Incorrect identically-labeled DataFrame objects Exception comes with df.compare method #50083 #50287

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 27, 2022
Merged

Incorrect identically-labeled DataFrame objects Exception comes with df.compare method #50083 #50287

merged 5 commits into from
Dec 27, 2022

Conversation

tpackard1
Copy link
Contributor

@tpackard1
Copy link
Contributor Author

First time contributing to the code base and hopefully everything goes smoothly.

@MarcoGorelli MarcoGorelli self-requested a review December 15, 2022 21:56
@tpackard1 tpackard1 changed the title Fixed Incorrect identically-labeled DataFrame objects Exception comes with df.compare method #50083 Incorrect identically-labeled DataFrame objects Exception comes with df.compare method #50083 Dec 16, 2022
MarcoGorelli
MarcoGorelli suggested changes Dec 16, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a nice start! could you also:

  • fixup the failing tests
  • add a whatsnew note

@MarcoGorelli MarcoGorelli added the Error Reporting Incorrect or improved errors from pandas label Dec 16, 2022
@tpackard1
Copy link
Contributor Author

I will try to fix the failing tests tonight.

@tpackard1
Copy link
Contributor Author

tpackard1 commented Dec 17, 2022 •
edited
Loading

@MarcoGorelli would the following work as a whatsnew note?
Whatsnew Note

  • Updated the Exception messages for df.compare method to include that both index and columns that are identically-labeled can be compared.
  • Updated the test associated with these Exceptions.

Edit: sorry for all the commits please see the most recent one with the message "fixed formatting"; if you would like I will try to get rid of the two before the last

@MarcoGorelli
Copy link
Member

You can just say that you improved the error message - I think there's some similar entries in the what's new file

No need to mention tests, it's a user facing note

@tpackard1
Copy link
Contributor Author

@MarcoGorelli I have updated the whats new file. Please review whenever you have a moment

MarcoGorelli
MarcoGorelli suggested changes Dec 19, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just got a minor comment on the whatsnew note

doc/source/whatsnew/v2.0.0.rst Outdated
@@ -97,6 +97,7 @@ Other enhancements
- Added :meth:`Index.infer_objects` analogous to :meth:`Series.infer_objects` (:issue:`50034`)
- Added ``copy`` parameter to :meth:`Series.infer_objects` and :meth:`DataFrame.infer_objects`, passing ``False`` will avoid making copies for series or columns that are already non-object or where no better dtype can be inferred (:issue:`50096`)
- :meth:`DataFrame.plot.hist` now recognizes ``xlabel`` and ``ylabel`` arguments (:issue:`49793`)
- :func:`DataFrame.compare` exception message thrown improved to include that both index and columns that are identically labeled can be compared. (:issue:`50083`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- :func:`DataFrame.compare` exception message thrown improved to include that both index and columns that are identically labeled can be compared. (:issue:`50083`)
- Improved error message in :func:`DataFrame.compare` to clarify that "identically labelled" refers to both index and columns (:issue:`50083`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the whatsnew note to match your comment above

MarcoGorelli
MarcoGorelli suggested changes Dec 19, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, something went wrong when merging/rebasing I presume, you've lost all your changes, all I can see if the whatsnew note change

@tpackard1
Copy link
Contributor Author

I have updated the whatsnew comment and added back in the all the other changes. I wanted to make sure all the checks were passed before requesting a review but some of the checks keep failing with the following error message ERROR - SystemError: initialization of _internal failed without raising an exception. Is this due to the change requested? Also sorry this is taking so much or your time @MarcoGorelli but I appreciate your time as this process as taught me alot.

@MarcoGorelli
Copy link
Member

Hey - CI failures are unrelated, they're showing up in other PRs, I'll see what's going on there

MarcoGorelli
MarcoGorelli suggested changes Dec 22, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last minor comment, sorry - then I think it's good to go 🚀

doc/source/whatsnew/v2.0.0.rst Outdated
@@ -98,6 +98,7 @@ Other enhancements
- Added :meth:`Index.infer_objects` analogous to :meth:`Series.infer_objects` (:issue:`50034`)
- Added ``copy`` parameter to :meth:`Series.infer_objects` and :meth:`DataFrame.infer_objects`, passing ``False`` will avoid making copies for series or columns that are already non-object or where no better dtype can be inferred (:issue:`50096`)
- :meth:`DataFrame.plot.hist` now recognizes ``xlabel`` and ``ylabel`` arguments (:issue:`49793`)
- Improved error message in :func:`DataFrame.compare` to clarify that "identically labelled" refers to both index and columns (:issue:`50083`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, just noticed that this affects much more than just compare, e.g:

In [8]: df == dfother
---------------------------------------------------------------------------
ValueError: Can only compare identically-labeled DataFrame objects

would also have thrown this error

let's rephrase as

Improved error message when trying to align :class:`DataFrame` objects (for example, in :func:`DataFrame.compare`) to clarify ...

MarcoGorelli
MarcoGorelli approved these changes Dec 23, 2022
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @tpackard1

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 23, 2022
@mroeschke mroeschke merged commit 029e098 into pandas-dev:main Dec 27, 2022
@mroeschke
Copy link
Member

Thanks @tpackard1

@tpackard1 tpackard1 deleted the test-build-test branch January 20, 2023 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke approved these changes

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees
No one assigned
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

Incorrect identically-labeled DataFrame objects Exception comes with df.compare method
3 participants
@tpackard1 @MarcoGorelli @mroeschke