-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Show shape any time it cannot be inferred in repr #27482
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
..., | ||
[[ 0.]]]])""") | ||
) | ||
try: |
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.
This change does two things: correctly put the printoptions setting inside a try/finally
, and ensure that there are never any spaces at the end of a line (my editor removed those, thus causing the tests to fail).
We're not going to change the default representation without a wider discussion on the mailing list and a NEP. This will cause a ton of churn for docstring examples in all downstream projects, so we won't pull the trigger on this without a significant consensus from a lot of stakeholders. I have my doubts that we'll get that kind of strong agreement at this time. NEP 51 was the last change of this kind, and it was significantly smaller in scope. In any case, since there is no |
2530d2d
to
20b2bab
Compare
Hmm, we do give the shape already for shapes that include zeros, and the Anyway, point taken that this should go by the mailing list (though if the consensus is that this requires a NEP, I will just close this...). |
This part does strike me as contrary to what a repr does, so would prefer the version with |
I don't mind this change. It seems right that this is far less impactful than the scalar repr change if it only kicks in when eval round-tripping can't possibly work (if we need a NEP, then just a tiny one for the process of agreeing to it, I think). |
Given the feedback on the mailing list, I tried implementing adding Current sample output
|
98e5bbf
to
4eb54b2
Compare
OK, I figured out the doctests - it is just very slow if something fails. Must admit that the comment after the array works less well when the array is part of a
But arguably rare enough that it doesn't matter much. I do like not having the |
I guess the whole point here is not to be disruptive, otherwise That There is no constructor for which it makes sense to derive shape from an input object AND specify the shape (ndarray comes closest, but takes a buffer instead of obj), so I suppose it's futile. Slightly radical, perhaps, but we could add a |
Yeah, while an |
Actually, not totally useless for Back to this PR, I find the comment inside the list rather bothersome too, and agree with @seberg that the "broken" syntax is not that bad - after all, it is only shown for cases where the data string itself could not be parsed anyway. Shall I go ahead and remove the last commit that tried it? p.s. I did have one further alternative, which would be to replace the |
I remember seeing this discussed as an issue or mailing list post at some point (i.e. someone felt strong about it). Personally, I tend to think it adds much visual clutter (maybe in an html repr where you can reduce the clutter). |
Summarizing the more difficult problem: changing the repr breaks doc tests. It seems there are about a dozen in scipy, but only a handful in NumPy. If we go ahead with this, we could recommend projects use |
What was the final verdict on where to have the shape information? As a "fake" argument (perhaps to be added later as a real one), or as the comment? If the former, I'll need to remove the last commit. |
I've warmed to the non-existent keyword argument, because of the above. |
Agreed. That was also the sentiment at the triage meeting: a fake argument not a comment. |
To help set oneself up for using shape for different reasons.
4eb54b2
to
c5f2516
Compare
OK, removed the last commit (and rebased), so it is back to a fake |
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.
LGTM. A short release note that mentions the legacy=2.1
fix for projects with many doctest failures should be added.
So that it is always there when the shape cannot be inferred from list
c5f2516
to
3c6d763
Compare
OK, thanks, I added a changelog snippet. |
Thanks @mhvk |
PSA note for projects using scipy-doctest: upgrade to scipy-doctest==1.5.1, update your doctests to use the new repr, and your doctesting will work on both numpy 2.2 and older versions. |
This change broke xarray's printed representation (xref pydata/xarray#9873) - thank you for providing an escape hatch, but is there a way for me to enable that as a global option? |
You can also use Although, in this case since it seems like a single test, maybe it is just as easy to adapt the test to allow both? |
As noted in #27461, when an array is summarized, the
repr
no longer makes it clear what the shape is. This is a bit odd, since for the case of a zero-sized array, the shape is shown when it is not clear what it would be (i.e., when the size is zero but the shape multi-dimensional). Similarly, if the dtype is not obvious, it is shown. So, this PR also shows the shape when summaries are made:One can get the old behaviour by using
legacy='2.1'
- I'm not sure this is really required; happy to remove it if desired.Fixes #27461
p.s. The first commit reorganizes the routine to make it easier to add the shape, while the second has the actual enhancement and tests.