REF/CLN: pandas/io/pytables.py #36859

ivanovmg · 2020-10-04T16:17:03Z

Clean-up and minor refactor of pandas/io/pytables.py.

Extract method _identify_group
Clean-up some docstrings (spacing)
Use suppress instead of try/except/pass
Format error message using dedent
Remove unnecessary empty lines as they compromise readability of some blocks

jbrockmendel · 2020-10-04T18:51:25Z

pandas/io/pytables.py

@@ -716,11 +712,9 @@ def open(self, mode: str = "a", **kwargs):
                    "which allows\n"
                    "files to be opened multiple times at once\n"
                )
-
            raise err

        except Exception as err:


if you're up for it, narrowing down what we catch here would be really helpful

I did that, looking at the source code of tables. But it turns out that there was (is) no test for this exception in pandas code. So I will need to figure out how to do it, but it will require some mocking, including package version and stuff.

Taking into account that pytables version >= 3.4.3 in current installation requirements (https://pandas.pydata.org/pandas-docs/stable/getting_started/install.html), then I can probably remove ver >= 3.1 statement.
In fact, the whole message can be updated to a more relevant one.

jbrockmendel · 2020-10-04T18:51:41Z

pandas/io/pytables.py

        where : list, default None
-                List of Term (or convertible) objects, optional.
+            List of Term (or convertible) objects, optional.
        start : int, default None


int -> int or None

Changed here and in the adjacent strings.

jbrockmendel · 2020-10-04T18:52:05Z

pandas/io/pytables.py

@@ -1099,7 +1091,7 @@ def put(
            <https://pandas.pydata.org/pandas-docs/stable/user_guide/io.html#query-via-data-columns>`__.
        encoding : str, default None
            Provide an encoding for strings.
-        dropna   : bool, default False, do not write an ALL nan row to
+        dropna : bool, default False, do not write an ALL nan row to


new line after "default False"

It turns out that this parameter is not even used in the function. Deleted the corresponding strings.

jbrockmendel · 2020-10-04T18:52:43Z

pandas/io/pytables.py

@@ -1807,6 +1771,45 @@ def _read_group(self, group: "Node"):
        s.infer_axes()
        return s.read()

+    def _identify_group(self, key: str, append: bool) -> "Node":
+        """Identify HDF5 group based on key, delete/create group if needed."""


""" on separate lines

No problem, but is there a local policy for this? I mean according to PEP257 (https://www.python.org/dev/peps/pep-0257/#one-line-docstrings) if it fits in one line, then no need to put triple quotes on multiple lines.

cc @datapythonista am i remembering this wrong?

For private docstrings it doesn't make a difference, and both are correct based on Python recommendations. For public docstrings we try to standardize them to what @jbrockmendel says. We shouldn't have single liners anyway, and in our autosummaries we take the first line of the docstring (the short summary), which we want to have of similar sizes to show correctly in the html version. Having it in the line after the """ allows for 3 more characters, so we decided to standardize all public docstrings to that, to be consistent.

jbrockmendel · 2020-10-04T18:54:17Z

pandas/io/pytables.py

-            for m in masks[1:]:
-                mask = mask & m
+            combine_masks = lambda first, second: first & second
+            mask = reduce(combine_masks, masks)


not a deal-breaker, but i dont find this clearer than what we have now

OK, I supposed it may be a questionable change. I will revert this.

jbrockmendel · 2020-10-04T18:55:13Z

pandas/io/pytables.py

@@ -5032,14 +5015,12 @@ def _maybe_adjust_name(name: str, version) -> str:
    -------
    str
    """
-    try:
+    with suppress(IndexError):


let's do appropriate checks for this and avoid the exception altogether

I checked that the version number is a correct sequence of three elements. I also added test as this check never hit.

Note that there are some other params that are not in docstring.

This reverts commit 65996e6.

ivanovmg · 2020-10-05T19:25:05Z

pandas/io/pytables.py

-            # trying to read from a non-existent file causes an error which
-            # is not part of IOError, make it one
-            if self._mode == "r" and "Unable to open/create file" in str(err):
-                raise OSError(str(err)) from err
-            raise


Looks like this error message is not raised in the current version of tables.file. Not even 3.1. So, I deleted this error raise.

ivanovmg · 2020-10-05T19:27:34Z

pandas/io/pytables.py

-        try:
-            self._handle = tables.open_file(self._path, self._mode, **kwargs)
-        except OSError as err:  # pragma: no cover
-            if "can not be written" in str(err):
-                print(f"Opening {self._path} in read-only mode")
-                self._handle = tables.open_file(self._path, "r", **kwargs)
-            else:
-                raise


I checked the current code of tables.open_file and did not find error messages containing can not be written. Even in 3.1 there is no such message. Considering the current minimal pytables version (>= 3.4.3), this error handing seems to be irrelevant (if "can not be written" in str(err)).

yeah this could have changed, was hard to test :->

jreback

looks fine, just a question.

jreback · 2020-10-06T03:53:20Z

pandas/io/pytables.py

-        try:
-            self._handle = tables.open_file(self._path, self._mode, **kwargs)
-        except OSError as err:  # pragma: no cover
-            if "can not be written" in str(err):
-                print(f"Opening {self._path} in read-only mode")
-                self._handle = tables.open_file(self._path, "r", **kwargs)
-            else:
-                raise


yeah this could have changed, was hard to test :->

jreback · 2020-10-06T03:54:19Z

pandas/io/pytables.py

-
-        # remove the node if we are not appending
-        if group is not None and not append:
-            self._handle.remove_node(group, recursive=True)


what happened to this?

I put it in the method _identify_group below.

jreback · 2020-10-06T22:44:28Z

thanks @ivanovmg

ivanovmg added 6 commits October 4, 2020 22:26

REF: extract method _identify_group

1a959e6

DOC: clean-up docstrings

1448d49

CLN: use suppress instead of try/except/pass

b166bb6

CLN: update multi-line message using dedent

c80b2d3

REF: reduce for combining mask pair-wise

65996e6

CLN: remove unnecessary empty lines

e59dc62

ivanovmg requested review from jreback and jbrockmendel October 4, 2020 16:38

jbrockmendel reviewed Oct 4, 2020

View reviewed changes

ivanovmg added 5 commits October 6, 2020 00:16

DOC: delete unused parameter from docstring

35f4d02

Note that there are some other params that are not in docstring.

DOC: None in docstrings

f5df4c0

REF: avoid IndexError check

98b9049

Revert "REF: reduce for combining mask pair-wise"

d98ab24

This reverts commit 65996e6.

LINT: fix quotes

c32086e

ivanovmg force-pushed the refactor/pytables branch from 7abe6f6 to c32086e Compare October 5, 2020 17:57

ivanovmg added 3 commits October 6, 2020 01:58

REF: narrow down reason for exception

571b965

REF: remove what seems to be irrelevant exceptions

262e9a0

CLN: update error handing with already opened file

aaf075d

ivanovmg commented Oct 5, 2020

View reviewed changes

ivanovmg requested a review from jbrockmendel October 5, 2020 19:59

jreback requested changes Oct 6, 2020

View reviewed changes

ivanovmg requested a review from jreback October 6, 2020 05:49

jreback added IO HDF5 read_hdf, HDFStore Refactor Internal refactoring of code labels Oct 6, 2020

jreback added this to the 1.2 milestone Oct 6, 2020

jreback approved these changes Oct 6, 2020

View reviewed changes

jreback merged commit 98af5d7 into pandas-dev:master Oct 6, 2020

ivanovmg deleted the refactor/pytables branch October 7, 2020 09:44

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020

REF/CLN: pandas/io/pytables.py (pandas-dev#36859)

8b78bb5

vnlitvinov mentioned this pull request Dec 10, 2020

test_HDFStore uses hardcoded absolute filename which breaks parallel test sessions modin-project/modin#2527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF/CLN: pandas/io/pytables.py #36859

REF/CLN: pandas/io/pytables.py #36859

ivanovmg commented Oct 4, 2020 •

edited

Loading

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

ivanovmg Oct 5, 2020 •

edited

Loading

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

jbrockmendel Oct 5, 2020

datapythonista Oct 5, 2020

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

jbrockmendel Oct 4, 2020

ivanovmg Oct 5, 2020

ivanovmg Oct 5, 2020

ivanovmg Oct 5, 2020

jreback Oct 6, 2020

jreback left a comment

jreback Oct 6, 2020

jreback Oct 6, 2020

ivanovmg Oct 6, 2020

jreback commented Oct 6, 2020

REF/CLN: pandas/io/pytables.py #36859

REF/CLN: pandas/io/pytables.py #36859

Conversation

ivanovmg commented Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanovmg Oct 5, 2020 • edited Loading

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

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

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

jreback commented Oct 6, 2020

ivanovmg commented Oct 4, 2020 •

edited

Loading

ivanovmg Oct 5, 2020 •

edited

Loading