-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
COMPAT: Update for NumPy dev #17987
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
COMPAT: Update for NumPy dev #17987
Conversation
return np.fromstring(values, dtype=dtype) | ||
# Copy the bytes into a numpy array. | ||
buf = np.frombuffer(values, dtype=dtype) | ||
buf = buf.copy() # required to not mutate the original data |
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 think it's safe to not copy here, IIRC values
is pile of bytes read from a msgpack file, so no risk in mutating?
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.
Without the copy the test failed:
for buf, control_buf in zip(not_garbage, control):
# make sure none of our mutations above affected the
# original buffers
> assert buf == control_buf
E AssertionError: assert b'\x00\x00\x0...x00\x00@\x8f@' == bytearray(b'\x...00\x008\x8f@')
E At index 6 diff: 240 != 0
E Use -v to get the full diff
pandas/tests/io/test_packers.py:690: AssertionError
I didn't look closely to see if that's a legitimate problem or not.
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.
Ah, in that case I would trust the test.
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 need to do this conditionally for numpy > 1.13 I think
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.
cc @llllllllll
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.
The tests on the older NumPy passed, let me make sure zlib is installed and that they weren't skipped.
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.
Pretty sure that https://travis-ci.org/pandas-dev/pandas/jobs/292845756 hit it, so I think we're OK.
Codecov Report
@@ Coverage Diff @@
## master #17987 +/- ##
==========================================
+ Coverage 91.23% 91.57% +0.33%
==========================================
Files 163 163
Lines 50113 53390 +3277
==========================================
+ Hits 45723 48892 +3169
- Misses 4390 4498 +108
Continue to review full report at Codecov.
|
Huh, must have deleted that branch accidentally. |
thanks! |
Closes pandas-dev#17986 xref numpy/numpy#9487 (cherry picked from commit dff5109)
Closes #17986 xref numpy/numpy#9487 (cherry picked from commit dff5109)
Closes #17986
xref numpy/numpy#9487
Do we want this for 0.21?