Skip to content
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 Assert_frame_equal check_names to True #2964

Merged
merged 2 commits into from Mar 7, 2013

Conversation

@hayd
Copy link
Contributor

commented Mar 4, 2013

This is a follow up to #2962, where I added a check_names=False argument to assert_frame_equal, this defaults it to True. That is, when asserting two DataFrames are equal, it compares .columns.names and .index.names, unless explicitly passing check_names=False.

I've added in check_names=False where it wouldn't make sense to check names. I've added a TODO comment (and check_names=False) where I think it probably should work but isn't... This was in the following cases:

to_csv
reindex
reset_index
groupy-a-get
pop
join (dataframe to a series)
from_xls (when setting index as first column)

i.e. these appear to drop column names (df.columns.names). Whether this behaviour is correct / desired in these cases, I'm not sure, but here they all are. :)

Note: By changing the A1 cells to 'index', I had all but one test (labelled "read xls ignores index name ?") in test_excel.py working locally (with check_names=True) , however Travis was less convinced throwing:

InvalidFileException: "There is no item named 'xl/theme/theme1.xml' in the archive"

so I reverted to the previous xls and xlsx, and added back in check_names=False. I suspect LibreOffice wasn't saving it properly...?

@hayd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2013

@wesm @changhiskhan @jreback Do you think this is ok to merge in?

I'm being a bit wary of changing the behaviour of assert_frame_equals if you're not onboard. (Since, it could lead to test passes merging into test fails, if assert_frame_equals is used in a test to frames with distinct column/index names.)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2013

any tests failing?

@hayd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2013

@jreback Nope it's "good to merge"..

@jreback

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2013

@hayd so you basically set check_names=False with a comment to check it.....

I guess maybe make a new issue refing this one....so you (or someone) can follow up in those cases.....they seem like the should work.....

@hayd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2013

The original PR #2962 does just that (sets it to False), this second one sets it True...

I had to go in and fixed those calls to assert_frame_equals where it wasn't working, either by setting check_names=False, changing the name (of an expected/result) or flagging it with TODO (and setting check_names=False) if I thought it could be a bug.

Then I (or someone) can then follow them up :)

@wesm

This comment has been minimized.

Copy link
Member

commented Mar 7, 2013

This looks fine to merge to me. I think it'll help catch bugs

hayd added a commit that referenced this pull request Mar 7, 2013
Merge pull request #2964 from hayd/assert_frame_equal_check_names
ENH Assert_frame_equal check_names to True

@hayd hayd merged commit 9a6810d into pandas-dev:master Mar 7, 2013

@dalejung

This comment has been minimized.

Copy link
Contributor

commented on be3d55d Mar 13, 2013

Could you put a note of this in the release notes? This broke a bunch of tests on my end.

This comment has been minimized.

Copy link
Member

replied Mar 15, 2013

+1

@hayd

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2013

Sorry about that, I've a line in release notes in the PR above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.