Skip to content

Conversation

@ajamian
Copy link
Contributor

@ajamian ajamian commented May 4, 2015

Closes #7212. If this object is a list, make a copy and then pass to process_axes - otherwise, just pass the object directly (in this case it is most likely none).

A test for this behavior seems complex and somewhat contrived when compared with what exists in test_pytables.py, so I didn't add one --- let me know any thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

Checking for specific types is generally a bad idea -- what if, for example, a user passes in a list subclass, or another sequence type?

A better approach is to cast any provided columns to a list:

if columns is not None:
    columns = list(columns)

However, given that the mutation happens in the process_axes method, it would be a better idea to put this there.

@shoyer
Copy link
Member

shoyer commented May 4, 2015

I know it seems somewhat contrived, but we do try to add tests even for things like this.

@shoyer shoyer added Bug IO HDF5 read_hdf, HDFStore labels May 4, 2015
@jreback jreback changed the title BUG: closes issue #7212 - read_hdf modifying passed columns list BUG: read_hdf modifying passed columns list (GH7212) May 4, 2015
@jreback jreback added this to the Next Major Release milestone May 4, 2015
@ajamian
Copy link
Contributor Author

ajamian commented May 4, 2015

@shoyer thanks for the feedback! much better solution. i'll get a test together as well.

@jreback
Copy link
Contributor

jreback commented May 14, 2015

@ajamian can you add a test for this?

@ajamian
Copy link
Contributor Author

ajamian commented May 15, 2015

@jreback absolutely, should be getting to it this weekend.

@ajamian
Copy link
Contributor Author

ajamian commented May 23, 2015

@shoyer @jreback thanks for the feedback --- let me know if you have any other comments

@shoyer
Copy link
Member

shoyer commented May 23, 2015

@ajamian one last thing -- can you add a note to the release notes for 0.17? otherwise this looks good to me!

@jreback
Copy link
Contributor

jreback commented May 23, 2015

yep lgtm as well - pls add a comment on the test with the issue number

@ajamian
Copy link
Contributor Author

ajamian commented May 26, 2015

I put the note into doc/source/whatsnew/v0.17.0.txt, I think this is where it belongs...and comment added to test. Thanks guys - let me know if you have any more comments.

shoyer added a commit that referenced this pull request May 26, 2015
BUG: read_hdf modifying passed columns list (GH7212)
@shoyer shoyer merged commit a9c2b71 into pandas-dev:master May 26, 2015
@shoyer
Copy link
Member

shoyer commented May 26, 2015

Thanks!

@jreback
Copy link
Contributor

jreback commented May 26, 2015

thanks Tom!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug IO HDF5 read_hdf, HDFStore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

read_hdf / store.select modifies the passed columns parameters when multi-indexed

3 participants