-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: groupby and agg on read-only array gives ValueError: buffer source array is read-only #36061
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
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 good, can you add a note in 1.1.2 fixed regression section.
also you are referencing 2 issues. can you add tests that are in these issues.
|
||
if len(values) != len(labels): | ||
if len_values != len_labels: |
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 elsewhere we get around this be using values.shape[0]
instead of len
. IIRC this is something we wont need to do anymore in cython3
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.
There are indeed a few cases of
# TODO(cython 3.0):
# Instead of `labels.shape[0]` use `len(labels)`
if not len(values) == labels.shape[0]:
in this file. But so if we need to update it anyways after cython 3.0, it doesn't maybe matter that much.
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.
yeah i think ok for now
LGTM pending Jeff's test request |
Tests for #35436 and #34857? Those issues are not fixed by this PR. Even if I add tests, they'd fail. Unless you mean something else which I am missing. I tried @jbrockmendel's suggestion. Also, should the |
|
||
if len(values) != len(labels): | ||
if len_values != len_labels: |
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.
yeah i think ok for now
thanks @jeet-parekh |
… gives ValueError: buffer source array is read-only
…ueError: buffer source array is read-only (#36117) Co-authored-by: Jeet Parekh <[email protected]>
…ce array is read-only (pandas-dev#36061)
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
I found multiple aggregate functions that were failing for a read-only array. I applied the change suggested in #36014 to all of them.
A couple of questions.
len(values) != len(labels)
. Let me know if there is another preferred way of doing it.len(values) != len(labels)
check. Should it be there in all the functions?