BUG: regression for array([pandas.DataFrame()]) #13663
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Fixes #13659 |
seberg
reviewed
May 29, 2019
There was a problem hiding this comment.
Seems OK to me as a reversion. I am not sure about the DECREF, I was wondering if it should have a comment that this is a bit odd, but then if its a 0 length sequence, the assignment should be empty in any case (of course Eric will create some malicious object which would bring it down in flames).
Fixes the regression hitting Pandas. But does not fix the real (never noticed) but that this gives the incorrect output shape to begin with.
numpy/core/src/multiarray/ctors.c
Outdated
| return 0; | ||
| } | ||
| else { | ||
| Py_DECREF(Py_NotImplemented); |
There was a problem hiding this comment.
Shouldn't this be an unconditional Py_DECREF(tmp)?
numpy/core/tests/test_regression.py
Outdated
| #gh-13659, would raise in broadcasting [x=t for x in result] | ||
| np.array([t]) | ||
|
|
||
| def test_2d__array__shape(self): |
There was a problem hiding this comment.
Seems like this whole code block is just duplicated?
725a9a5 to
af094ef
Compare
seberg
reviewed
May 29, 2019
| goto fail; | ||
| } | ||
| else if (tmp != Py_NotImplemented) { | ||
| if (PyArray_CopyInto(dst, (PyArrayObject *)tmp) < 0) { | ||
| goto fail; |
There was a problem hiding this comment.
Suggested change
| goto fail; | |
| Py_DECREF(tmp); | |
| goto fail; |
Something is still off here with the reference counting. Also the return 0 below needs it. Which probably means the whether or not the else clause is there did not matter much. Or we make tmp = NULL; one scope up to make the fail paths nicer.
There was a problem hiding this comment.
Thanks, looks right now. Will merge as soon as tests finish (or squash merge).
eric-wieser
reviewed
May 31, 2019
|
|
||
| t = T() | ||
| #gh-13659, would raise in broadcasting [x=t for x in result] | ||
| np.array([t]) |
There was a problem hiding this comment.
Can we add a test here for the shape?
TomAugspurger
added a commit
to TomAugspurger/pandas
that referenced
this pull request
May 31, 2019
Reverts pandas-dev#26548 xref numpy/numpy#13663 Closes pandas-dev#26546
TomAugspurger
added a commit
to pandas-dev/pandas
that referenced
this pull request
May 31, 2019
Reverts #26548 xref numpy/numpy#13663 Closes #26546
seberg
added a commit
to seberg/numpy
that referenced
this pull request
Jul 24, 2019
…rray-like" This reverts commit 71fc59d which was part of numpygh-13399 and the followup commit ba53a63 from numpygh-13663. The issue is that we mix Sequence protocol and `__array__` usage when coercing arrays. It seems easiest to revert this for the 1.17.x release and make a larger breaking change in the 1.18.x release cycle. Since this only occurs for stranger array-likes. This revert is limited to the 1.17.x branch for now.
charris
pushed a commit
to charris/numpy
that referenced
this pull request
Nov 26, 2019
…rray-like" This reverts commit 71fc59d which was part of numpygh-13399 and the followup commit ba53a63 from numpygh-13663. The issue is that we mix Sequence protocol and `__array__` usage when coercing arrays. It seems easiest to revert this for the 1.17.x release and make a larger breaking change in the 1.18.x release cycle. Since this only occurs for stranger array-likes. This revert is limited to the 1.17.x branch for now.
charris
pushed a commit
to charris/numpy
that referenced
this pull request
Dec 2, 2019
…rray-like" This reverts commit 71fc59d which was part of numpygh-13399 and the followup commit ba53a63 from numpygh-13663. The issue is that we mix Sequence protocol and `__array__` usage when coercing arrays. It seems easiest to revert this for the 1.18.x release and make a larger breaking change in the 1.19.x release cycle. Since this only occurs for stranger array-likes.
seberg
added a commit
to seberg/numpy
that referenced
this pull request
Feb 6, 2020
``__array__`` was previously not used during dimension discovery, while bein gused during dtype discovery (if dtype is not given), as well as during filling of the resulting array. This would lead to inconsistencies with respect to array likes that have a shape including a 0 (typically as first dimension). Thus a shape of ``(0, 1, 1)`` would be found as ``(0,)`` because a nested list/sequence cannot represent empty shapes, except 1-D. This uses the `_array_from_array_like` function, which means that some coercions may be tiny bit slower, at the gain of removing a lot of complex code. (this also reverts commit d0d250a or the related change). This is a continuation of work by Sergei Lebedev in numpygh-13663 which had to be reverted due to problems with Pandas, and the general inconsistency. This version may not resolve all issues with pandas, but does resolve the inconsistency. Closes numpygh-13958
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13399
Add failing test.
Fix by not converting 0-len object to ndarray, which is compatible with behaviour before #13399