BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include #32723

roberthdevries · 2020-03-15T10:44:05Z

closes HDF5: empty groups and keys #29916
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2020-03-15T10:53:04Z

Hello @roberthdevries! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-14 21:02:12 UTC

jreback

looks fine. does this solve the OP? meaning folks simply want to use .keys() to get all the tables, then use .get() to read them?

jreback · 2020-03-16T02:35:34Z

pandas/io/pytables.py

+        if include == "pandas":
+            return [n._v_pathname for n in self.groups()]
+
+        if include == "native":


Not really needed in this case as the previous if returns in all cases. Why would that help us?

can you change this (its more idiomatic)

pandas/tests/io/pytables/test_store.py

pandas/io/pytables.py

jreback · 2020-03-16T02:39:05Z

pandas/io/pytables.py

@@ -580,16 +580,37 @@ def __enter__(self):
    def __exit__(self, exc_type, exc_value, traceback):
        self.close()

-    def keys(self) -> List[str]:
+    def keys(self, include: Optional[str] = "pandas") -> List[str]:


this is not Optional, just str (alternatively, we could actually do Optiona[str] = None and then allow passing include='native')

but find either way

Removed the Optional annotation

WillAyd · 2020-03-18T02:15:15Z

pandas/io/pytables.py

+        Parameters
+        ----------
+
+        .. versionadded:: 1.1.0


Same comment on other PR - put below include and indent one level (I think will fix CI)

roberthdevries · 2020-03-19T19:53:54Z

looks fine. does this solve the OP? meaning folks simply want to use .keys() to get all the tables, then use .get() to read them?

We could also merge the two sets of tables. I think pandas does not use native HDF5 tables to store DataFrames. In that case the two sets are disjoint and we should be fine returning the joined set.

roberthdevries · 2020-03-19T20:19:01Z

I tried the variant with returning the combination of keys of pandas tables and HDF5 native tables, and the unit test pandas/tests/io/pytables/test_store.py::TestHDFStore::test_copy fails. I think the solution we have now helps the dask developers at least to have a work-around solution, that can work on both older and newer pandas versions by checking for the pandas version.

st-bender · 2020-05-09T20:33:46Z

I tried the variant with returning the combination of keys of pandas tables and HDF5 native tables, and the unit test pandas/tests/io/pytables/test_store.py::TestHDFStore::test_copy fails.

Hi there, this is interesting, how did this test pass before the keys() method changed, i.e. it used to return all the keys in pandas < 0.24?

I think the solution we have now helps the dask developers at least to have a work-around solution, that can work on both older and newer pandas versions by checking for the pandas version.

Hopefully, thanks for working on it.

jreback

looks fine, pls merge master and ping on green

jreback · 2020-06-14T15:32:48Z

pandas/io/pytables.py

+        if include == "pandas":
+            return [n._v_pathname for n in self.groups()]
+
+        if include == "native":


can you change this (its more idiomatic)

An additional kind parameter has been added that defaults to pandas original behavior, but with 'tables' value gives you the list of non-pandas tables in the file

- improve type annotation of the HDFStore.keys method - minor improvement in ValueError string - minor improvement in doc-string

…ameter value

jreback

don't you need this also in .groups()?

jreback · 2020-06-14T20:34:58Z

pandas/io/pytables.py

+        include : str, default 'pandas'
+                When kind equals 'pandas' return pandas objects
+                When kind equals 'native' return native HDF5 Table objects
+                Otherwise fail with a ValueError


this line is not needed (the otherwise fail)

roberthdevries · 2020-06-14T21:08:47Z

don't you need this also in .groups()?

Could be added there as well, although that use case is less typical. Most users are interested in the tables in the file.
However, if this question pops up, I'll be happy to create a PR for that.

roberthdevries · 2020-06-14T21:46:43Z

@jreback tests should have run fine, except that one of the tests took too long I think. So this is my ping on sort of green

jreback · 2020-06-14T22:20:23Z

thanks @roberthdevries

jamesbooker · 2020-06-15T07:47:45Z

thanks @roberthdevries and @jreback for resolving this issue for keys, it will help my project a lot.

@roberthdevries the source files coming into my system use groups (not created by pandas) so I would need the same fix applying to .groups() as well as .keys() if at all possible.

roberthdevries mentioned this pull request Mar 15, 2020

HDFStore: Fix empty result of keys() method on non-pandas hdf5 file #32401

Closed

5 tasks

roberthdevries changed the title ~~Fix 29916 by adding keyword include~~ Fix HDFStore empty keys on native HDF5 file by adding keyword include Mar 15, 2020

jreback added IO HDF5 read_hdf, HDFStore API - Consistency Internal Consistency of API/Behavior labels Mar 16, 2020

jreback requested changes Mar 16, 2020

View reviewed changes

st-bender mentioned this pull request Mar 17, 2020

dd.read_hdf fails with pandas >= 0.24.0 dask/dask#5934

Closed

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch from 025a348 to 001be86 Compare March 17, 2020 20:59

roberthdevries requested a review from jreback March 17, 2020 21:12

WillAyd reviewed Mar 18, 2020

View reviewed changes

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch from 3e114be to 020fc5b Compare March 18, 2020 19:51

roberthdevries changed the title ~~Fix HDFStore empty keys on native HDF5 file by adding keyword include~~ BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include Mar 18, 2020

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch 3 times, most recently from 4ee5d7e to ba1a8ca Compare March 24, 2020 20:42

roberthdevries requested a review from WillAyd March 24, 2020 21:24

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch 2 times, most recently from 83bce97 to 151aa4f Compare April 2, 2020 12:24

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch from 151aa4f to 8ce3475 Compare April 19, 2020 12:31

martindurant mentioned this pull request May 13, 2020

Cope with missing HDF keys dask/dask#6204

Merged

jreback requested changes Jun 14, 2020

View reviewed changes

roberthdevries added 6 commits June 14, 2020 22:19

HDFStore: Fix empty result of keys() method on non-pandas hdf5 file

22808a0

An additional kind parameter has been added that defaults to pandas original behavior, but with 'tables' value gives you the list of non-pandas tables in the file

Flake8 fixes

e023d11

black reformatter

efe00f6

Add whatsnew entry

df9c536

Correct the order of the keywords in the docstring

f7c9c32

Minor cleanups

4f1ccd6

- improve type annotation of the HDFStore.keys method - minor improvement in ValueError string - minor improvement in doc-string

roberthdevries added 11 commits June 14, 2020 22:19

Make test code for the non-pandas case a bit more explicit

64bb567

Changed new keyword parameter 'kind' to 'include'

5dbaa4c

Add test to check the error path for an illegal include keyword par…

69bd5b6

…ameter value

Fix pylint error

c39fc76

Remove Optional from include parameter

b590f53

Add versionadded tag

91c81af

Added .get to test

fab73a7

Fix versionadded tag 3rd try

f1ce709

Fix docstring empty lines

4ef7b87

Linting: Fix space at the start at the string

8969623

More idiomatic python

51d632d

roberthdevries force-pushed the fix-29916-by-adding-keyword-include branch from 8ce3475 to 51d632d Compare June 14, 2020 20:21

jreback added this to the 1.1 milestone Jun 14, 2020

jreback requested changes Jun 14, 2020

View reviewed changes

Remove "Otherwise fail with a ValueError" as requested

0f5dd68

jreback merged commit 4f625a2 into pandas-dev:master Jun 14, 2020

roberthdevries deleted the fix-29916-by-adding-keyword-include branch June 16, 2020 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include #32723

BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include #32723

roberthdevries commented Mar 15, 2020

pep8speaks commented Mar 15, 2020 •

edited

Loading

jreback left a comment

jreback Mar 16, 2020

roberthdevries Mar 17, 2020

jreback Jun 14, 2020

roberthdevries Jun 14, 2020

jreback Mar 16, 2020

roberthdevries Mar 17, 2020

WillAyd Mar 18, 2020

roberthdevries Mar 18, 2020

roberthdevries commented Mar 19, 2020

roberthdevries commented Mar 19, 2020

st-bender commented May 9, 2020

jreback left a comment

jreback Jun 14, 2020

jreback left a comment

jreback Jun 14, 2020

roberthdevries Jun 14, 2020

roberthdevries commented Jun 14, 2020

roberthdevries commented Jun 14, 2020

jreback commented Jun 14, 2020

jamesbooker commented Jun 15, 2020

BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include #32723

BUG: Fix HDFStore empty keys on native HDF5 file by adding keyword include #32723

Conversation

roberthdevries commented Mar 15, 2020

pep8speaks commented Mar 15, 2020 • edited Loading

Comment last updated at 2020-06-14 21:02:12 UTC

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberthdevries commented Mar 19, 2020

roberthdevries commented Mar 19, 2020

st-bender commented May 9, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberthdevries commented Jun 14, 2020

roberthdevries commented Jun 14, 2020

jreback commented Jun 14, 2020

jamesbooker commented Jun 15, 2020

pep8speaks commented Mar 15, 2020 •

edited

Loading