Skip to content

Clarify equals method docstring #19213

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
wants to merge 2 commits into from
Closed

Clarify equals method docstring #19213

wants to merge 2 commits into from

Conversation

gsganden
Copy link

@gsganden gsganden commented Jan 12, 2018

Make explicit that equals method requires that columns have the same dtypes but not that indices have the same types (e.g. pd.DataFrame({1:[0], 0:[1]}).equals(pd.DataFrame({1.0:[0], 0.0:[1]})) returns True while pd.DataFrame({1:[0], 0:[1]}, dtype='float32').equals(pd.DataFrame({1:[0], 0:[1]}, dtype='float64')) returns False)

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Make explicit that equals method requires that columns have the same dtypes but not that indices have the same types (e.g. `pd.DataFrame({1:[0], 0:[1]}).equals(pd.DataFrame({1.0:[0], 0.0:[1]}))` returns `True` while `pd.DataFrame({1:[0], 0:[1]}, dtype='float32').equals(pd.DataFrame({1:[0], 0:[1]}, dtype='float64'))` returns `False`)
@codecov
Copy link

codecov bot commented Jan 13, 2018

Codecov Report

Merging #19213 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19213      +/-   ##
==========================================
+ Coverage   91.53%   91.55%   +0.02%     
==========================================
  Files         147      147              
  Lines       48797    48797              
==========================================
+ Hits        44664    44675      +11     
+ Misses       4133     4122      -11
Flag Coverage Δ
#multiple 89.92% <ø> (+0.02%) ⬆️
#single 41.6% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/generic.py 95.9% <ø> (ø) ⬆️
pandas/core/dtypes/cast.py 88.35% <0%> (-0.31%) ⬇️
pandas/core/internals.py 94.5% <0%> (ø) ⬆️
pandas/core/dtypes/common.py 94.58% <0%> (+0.04%) ⬆️
pandas/plotting/_converter.py 66.95% <0%> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8347ff8...3bc9bfd. Read the comment docs.

@jreback jreback added the Docs label Jan 13, 2018
@jreback jreback added this to the 0.23.0 milestone Jan 13, 2018
@gsganden
Copy link
Author

Sorry, I meant "column labels" rather than "indices."

@jreback
Copy link
Contributor

jreback commented Jan 13, 2018

We check the indices for equality. But we happen to have this case.

In [7]: a
Out[7]: Int64Index([1, 0], dtype='int64')

In [8]: b
Out[8]: Float64Index([1.0, 0.0], dtype='float64')

In [9]: a.equals(b)
Out[9]: True

@jreback
Copy link
Contributor

jreback commented Jan 13, 2018

We normally DO consider that case equal, but I think we discussed in some other issues that from a user point of view these are not equal.

cc @toobaz @TomAugspurger @jorisvandenbossche

@jreback jreback removed this from the 0.23.0 milestone Jan 13, 2018
@toobaz
Copy link
Member

toobaz commented Jan 14, 2018

Without having thought much about this, I would have said that the indexes case (can be discussed, but mostly) is fine, while pd.DataFrame({1:[0], 0:[1]}, dtype='float32').equals(pd.DataFrame({1:[0], 0:[1]}, dtype='float64')) should have returned True...

but assuming I'm wrong (i.e. current behavior is OK), then I just suggest replacing the same dtypes at corresponding locations with the same dtype(s) in the patch.

@datapythonista
Copy link
Member

Closing this PR as discontinued. It's not in a state to be merged, and the user deleted the fork/branch it's based on.

Opened #22462 as I think it's better to start from the beginning on this docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants