-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix ValueError when reading a Dataframe with HDFStore in Python 3 fro… #24510
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
Conversation
…m fixed format written in Python 2
Codecov Report
@@ Coverage Diff @@
## master #24510 +/- ##
=======================================
Coverage 31.89% 31.89%
=======================================
Files 166 166
Lines 52421 52421
=======================================
Hits 16722 16722
Misses 35699 35699
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24510 +/- ##
==========================================
- Coverage 31.89% 31.88% -0.02%
==========================================
Files 166 166
Lines 52421 52426 +5
==========================================
- Hits 16722 16717 -5
- Misses 35699 35709 +10
Continue to review full report at Codecov.
|
so need a sample hdf file that we can include in the repo, generated in py2, that fails on reading (make a new test), and works with your patch. you can put it in here: pandas/tests/io/data/legacy_hdf read it like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments, would also need a whatsnew note
…xed format written in Python 2
@@ -2661,6 +2661,8 @@ def read_index_node(self, node, start=None, stop=None): | |||
|
|||
if 'name' in node._v_attrs: | |||
name = _ensure_str(node._v_attrs.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just directly call this, no if is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pandas/tests/io/test_pytables.py
Outdated
datapath('io', 'data', 'legacy_hdf', 'legacy_table_fixed_py2.h5'), | ||
mode='r') as store: | ||
with catch_warnings(): | ||
simplefilter("ignore", pd.io.pytables.IncompatibilityWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this come up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy / past error, this filter is useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
doc/source/whatsnew/v0.24.0.rst
Outdated
@@ -1515,6 +1515,7 @@ MultiIndex | |||
I/O | |||
^^^ | |||
|
|||
- Fix ``ValueError`` when reading a Dataframe with HDFStore in Python 3 from fixed format written in Python 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add the issue number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Add issue number to whatsnew
Try to decode index name systematically
Remove useless simplefilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok, some minor comments, you have some lint issue: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=6198
doc/source/whatsnew/v0.24.0.rst
Outdated
@@ -1515,6 +1515,7 @@ MultiIndex | |||
I/O | |||
^^^ | |||
|
|||
- Fix ``ValueError`` when reading a Dataframe with HDFStore in Python 3 from fixed format written in Python 2 (:issue:`24510`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move below (e.g. add it right before Plotting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry but I didn't understand your request, I'm not familiar with rst format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I got It, I moved my entry in the file
@@ -4540,6 +4540,18 @@ def test_pytables_native2_read(self, datapath): | |||
d1 = store['detector'] | |||
assert isinstance(d1, DataFrame) | |||
|
|||
def test_legacy_table_fixed_format_read_py2(self, datapath): | |||
# legacy table with fixed format written en Python 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the issue number here as a comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Change whatsnew category
pandas/tests/io/test_pytables.py
Outdated
datapath('io', 'data', 'legacy_hdf', | ||
'legacy_table_fixed_py2.h5'), | ||
mode='r') as store: | ||
with catch_warnings(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the warnings from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just used test_legacy_table_read(...) as a template to read the file. I removed this context manager locally and the test is still ok. I'll commit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great
lgtm @faulaire ping on green. |
thanks @faulaire |
Thanks for the review & commit @jreback |
Pull request to solve : 24404
git diff upstream/master -u -- "*.py" | flake8 --diff