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

DOC: Remove auto_close option form read_hdf #10330

Closed
wants to merge 1 commit into from

Conversation

bashtage
Copy link
Contributor

Remove docstring indicating auto_close can be used in read_hdf.
This value is always ignored.
Also removes unreachable code.

xref #9327

@jreback
Copy link
Contributor

jreback commented Jun 11, 2015

hmm, this should be hit in the iterator, or if you pass an open store to read_hdf. Maybe not tested.

@bashtage
Copy link
Contributor Author

It doesn't have a return so I don't think it can't do anything. Maybe I'm missing the point. Tests pass with it removed.

There is also another problem if a buffer is passed - there is no code path for a buffer since the if only checks for basestring and there is no else

@bashtage
Copy link
Contributor Author

I could probably improve it to handle an open store (although why one would want to use read_hdf if they have a store open is not obvious). The path to handle a buffer should be fairly straightforward (for example, if reading a HDF file from the internet). Presumably no one has ever tried to do this using read_hdf

@jreback
Copy link
Contributor

jreback commented Jun 11, 2015

I think it should handle an open store (and not close it), that is the key. But of its not tested. So for now, let's put a test that hits that path (and you can raise NotImplemented or make it work), though the object has to be an actual HDFStore (or then its a ValueError).

Since this hasn't been hit in real code, either it works (maybe) or raises some weird error. Either way its not tested.

@jreback jreback added the IO HDF5 read_hdf, HDFStore label Jun 11, 2015
@bashtage
Copy link
Contributor Author

It now checks and also raises if a generic buffer type is passed.

Added test for each.

@@ -4669,6 +4669,49 @@ def test_to_hdf_with_object_column_names(self):
result = pd.read_hdf(path, 'df', where="index = [{0}]".format(df.index[0]))
assert(len(result))

def test_read_hdf_open_store(self):
df = DataFrame(np.random.rand(4, 5),
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put the issues in a comment here

@jreback jreback added Error Reporting Incorrect or improved errors from pandas Clean labels Jun 11, 2015
@jreback jreback added this to the 0.16.2 milestone Jun 11, 2015
@jreback
Copy link
Contributor

jreback commented Jun 11, 2015

lgtm. minor comment ping when green.

@bashtage bashtage force-pushed the remove-auto-close branch 3 times, most recently from 2c47156 to 3245c00 Compare June 11, 2015 20:42
Remove docstring indicating auto_close can be used in read_hdf.
This value is always ignored.
Also correctly implements support for passing open HDFStores.
Adds error handling for unknown buffer-types.

xref pandas-dev#9327
jreback pushed a commit that referenced this pull request Jun 12, 2015
Remove docstring indicating auto_close can be used in read_hdf.
This value is always ignored.
Also correctly implements support for passing open HDFStores.
Adds error handling for unknown buffer-types.

xref #9327
@jreback
Copy link
Contributor

jreback commented Jun 12, 2015

merged via 51047d4

@jreback jreback closed this Jun 12, 2015
@bashtage bashtage deleted the remove-auto-close branch June 15, 2015 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Error Reporting Incorrect or improved errors from pandas IO HDF5 read_hdf, HDFStore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants