-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
ENH: Update scalar representations as per NEP 51 #22449
Conversation
342b701
to
ff552fc
Compare
@seberg so far so good for latest
|
So, I realized that the empty format string I don't really have an opinion, could even say that EDIT: Reconsidering this, maybe The thing to look into now (besides test fixes), is that array float formatting doesn't match scalar float formatting, so I actually need to pass through the formatting information better (should not be hard, but needs a bit organizing/thought.) |
893d77d
to
387e26a
Compare
This is starting to settle, although I am considering changing @rossbar do you know why the refguide check does not seem to notice changes from |
ce4166f
to
9a64758
Compare
I saw #22261 only now, so I won't complain if this is too late, but I'd really like to get away from |
I am proposing to print the bit-sized version, even if imprecise sometimes. Yes, it would be nice to change it, but it seems like a big ABI change on the C-side? |
Looked more closely, looks fine.
I'm afraid so, but I still think it'd be worth it eventually (numpy 2.0 material?). |
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 seems to be a mismatch between the implementation and the NEP w.r.t. whether printing is affected or not.
Based on the implementation, I think this PR has pretty high chance of disrupting services where something gets calculated with numpy, and then passed on in some serialized form to another consumer, where numpy (or even python) might not necessarily be installed.
Is there any fallback we could provide so that people can adapt such interfaces (e.g. .value
or .raw
or ...)?
It won't, as long as |
Needs rebase. |
decdee0
to
08c07ce
Compare
08c07ce
to
bf92d6e
Compare
@mhvk , how do you think |
7ea85db
to
7fcf5ca
Compare
8bdf97c
to
19ed59e
Compare
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.
@seberg - I think this all makes sense now. The only slightly larger remaining comment is about repr_format
and str_format
-- see there.
It may be good to have a follow-up issue about remaining items. It seems to be current array2string
is actually fine, but the question seems to be whether there should be some kind of option where all scalars, including void
, would round-trip. If we want to support sub-arrays for void
, then I think this would imply that having a round-trip version for array
would be a simple extension. But it seems to me it would also be fine not to do that - the main goal, of ensuring scalars are recognizable, has been reached here. Anyway, for a separate issue, let's get this one in first!
numpy/core/arrayprint.py
Outdated
@@ -536,7 +544,7 @@ def _array2string(a, options, separator=' ', prefix=""): | |||
summary_insert = "" | |||
|
|||
# find the right formatting function for the array | |||
format_function = _get_format_function(data, **options) | |||
format_function = _get_format_function(data=data, **options) |
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.
Might as well avoid unnecessary changes...
@@ -1400,13 +1408,23 @@ def __call__(self, x): | |||
return "({})".format(", ".join(str_fields)) | |||
|
|||
|
|||
def _void_scalar_repr(x): | |||
def _void_scalar_to_string(x, is_repr=True): |
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.
Yes, the name change is fine!
numpy/core/tests/test_arrayprint.py
Outdated
normalized_name = np.dtype(f"{dtype.kind}{dtype.itemsize}").type.__name__ | ||
assert representation == f"np.{normalized_name}({repr_string})" | ||
|
||
np.set_printoptions(legacy="1.25") |
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.
Fine in itself, but context manager safer:
with np.printoptions(legacy="1.25"):
...
numpy/core/tests/test_arrayprint.py
Outdated
assert repr(scalar) == representation | ||
|
||
np.set_printoptions(legacy="1.25") | ||
assert repr(scalar) == legacy_repr |
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.
Again would use context manager.
numpy/core/tests/test_longdouble.py
Outdated
@@ -316,7 +316,7 @@ class TestCommaDecimalPointLocale(CommaDecimalPointLocale): | |||
|
|||
def test_repr_roundtrip_foreign(self): |
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.
And here too.
@@ -232,6 +232,14 @@ found `here <https://github.com/numpy/numpy/pull/22449>`_ | |||
Implementation | |||
============== | |||
|
|||
.. note:: | |||
This part has *not* been implemented with the initial changes. |
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.
Might as well give a PR number, for easier reference: gh-22449.
To be honest, I still think we will eventually go back to what I had, although hopefully it's OK to dump the special special fmt states (and improved) (and just have Structured void repr should stay integrated with array repr, I think. So any changes there need to be integrated into |
Can we push this over the finish line? Yes, my doctest changeset isn't quite there yet, but not sure it will be helpful to wait, I will expect that the worst affected projects would use the |
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.
@seberg - sorry, lots of other stuff came up. But yes, this is good to go as far as I'm concerned!
Thanks Sebastian. |
In numpygh-22449 `descr` was renamed `dtype`, which broke the build as numpygh-24211 decrefed the old variable to fix a reference leak. This bug slipped in because numpygh-22449 was not retested after the merge.
So far I had to change it to |
Looks like we're headed to green with |
I will note that just setting the legacy mode for the tests will unfortunately hide issues if you use |
This is a WIP for implementing NEP 51/gh-22261. There are a few things that go beyond just changing the scalar representation:
It makes
get_formatter()
a more central/public API, to also movefmt=...
to it.dtype
is known, we can e.g. represent a longdouble as'3.1'
(with quotes) rather thannp.longdouble('3.1')
.fmt
is mainly used forfmt="r"
(previous point), I thinkfmt="s"
should be supported, but always be identical tostr(arr[0])
.fmt
path to be a ufunc (it should not inspect array values, unlike the default printing).fmt
should be used more cleanly for the__format__
work. This should be fine, I think it only means pushing the "option" adjustments lower.arr.tofile()
usedrepr(arr.item(...))
to print each element. This is weird/problematic:np.longdouble('3.1')
.'string'
orb'string'
Record and MA printing has to be adjusted to use the new functionality. This affects record scalars and the printing of the masked array
fill_value
.After some back and forth, I think the default should just be to go back to
str()
(this was changed silently in BUG 4381 Longdouble from string without precision loss #6199).str(arr.item())
is not ideal forfloat32
for example.This could use some thought (or should change to use the above
get_formatter(fmt="s")
, but I hope this can be decoupled from NEP 51.I plan to update the NEP 51 draft to include these points. Happy to discuss here of course (will make pushing NEP 51 quicker). But for now the draft-PR exists primarily to help with the NEP 51 discussion.
TODOS::
legacy
print-mode (i.e. to allow inclusion in the next release, but only switch with NEP 50 – which would likely be a NumPy 2.0 as well)arr.tofile()
is not well tested (which is why the change here should not fail tests).