ENH: show dtype in array repr when endianness is non-native by molsonkiko · Pull Request #23295 · numpy/numpy

molsonkiko · 2023-02-27T23:33:41Z

See #19059.
Goal is to get

>>> import numpy as np
>>> np.array([1], dtype='>u2') # nonstandard endianness should be reflected in repr
array([1], dtype='>u2')
>>> np.array([1], dtype='<u2')
array([1], dtype=uint16)

Try to fix problem where, for example, np.array([1], dtype='>u2') and np.array([1], dtype='<u2') both got represented as np.array([1], dtype=uint16)

seberg · 2023-02-28T21:14:51Z

    if dtype.names is not None:
        return False
+
+    if dtype.byteorder != '=' and dtype.itemsize != 1:


Tests are failing, I suspect dtype.isnative should work better?

That works much better! I tried running numpy/core/tests/test_arrayprint.py with the changes in my last commit and now all the tests in that file pass.

@seberg

@seberg pointed out that dtype.isnative could be used in place of my previous composite check. Now all tests in core/tests/test_arrayprint.py pass (except the one xfail)

This reverts commit eeaa2a9. I wanted to fix the linter errors, but reformatting with shed didn't help, so I'll just do it manually.

eric-wieser · 2023-03-04T01:52:51Z

+        # deal with cases like dtype('<u2') that are identical to an
+        # established dtype (in this case uint16)
+        # except that they have a different endianness.
+        return repr(dtype.descr[0][1])


Using descr feels like a mistake to me; descr is generally poorly behaved. I think there should be another way to get this information?

_construction_repr(..., short=True) from https://github.com/numpy/numpy/blob/main/numpy/core/_dtype.py looks like it might do the right thing.

I'll look into that. descr was a very ad-hoc choice made because it looked like the right kind of thing.

Yeah, the default in this branch looks like using str() or repr() one of them should be right (plausibly both, because complicated dtypes are dealt with earlier).

Given that all tests pass when using _construction_repr(dtype) in that conditional branch (the current status as of my most recent commit), should I leave as is, or do you think it's better to switch back?

That seems still very roundabout (maybe it is actually more direct, but using more internals), and str is already being used for structured dtypes?

eric-wieser · 2023-03-04T01:54:30Z

            array(['1', '1', '1', '1', '1', '1', '1', '1', '1', '1', '1', '1'],
                  dtype='{}')""".format(styp)))

+    @pytest.mark.parametrize(


Won't this test fail on big-endian systems?

Yep, good point.
I think what I'll do is

rename little_end to native and big_end to non_native

import sys if sys.byteorder == 'big': # flip native and non-native for big-endian systems non_native = non_native.replace('>', '<') native = native.replace('<', '>')

1. per eric-wieser's suggestion, using _construction_repr(dtype) instead of dtype.descr[0][1] to determine how to show non-native dtypes 2. Fix tests so they don't crash on big-endian systems. 3. Remove '<U' and '>U' tests because those are coerced into '<U1' and '>U1' respectively by np.array.

tylerjereddy

I tried testing on one of the compile farm Big Endian machines, but ran into some toolchain issues unrelated to the PR (ancient gcc version, etc.).

tylerjereddy · 2023-03-04T22:04:31Z

        '''
-        big_dtype = np.dtype(big_end)
-        little_dtype = np.dtype(little_end)
+        import sys


This import is already done at the top of the module

tylerjereddy · 2023-03-04T22:05:19Z

+            non_native = non_native.replace('>', '<')
+            native = native.replace('<', '>')
+        big_dtype = np.dtype(non_native)
+        little_dtype = np.dtype(native)


I suppose these variable names might get pretty confusing if the test is to be considered agnostic to endianness.

tylerjereddy · 2023-03-04T22:39:28Z

+                 '(e.g., int32, bool, float64) '
+                 'should not show dtype, but it does'))
+        # only show difference if > 1 byte
+        assert_(big_end_repr != little_end_repr or big_dtype.itemsize <= 1,


Maybe prefer plain assert for new testing code for the pytest augmentations?

molsonkiko · 2023-03-10T18:09:14Z

Is there more that I need to do to move this forward, or are you all just occupied with more pressing things right now?

I see that some of the build tests are failing, but I'm really not familiar with the failure modes exhibited, and it's not clear what I would do to address those issues.

mattip · 2023-03-12T22:47:32Z

close/reopen to restart circleci, which seems to have a temporary failure.

seberg

Just retriggered CircleCI once more. We should make sure it is happy, since this might have modified output.

The tests could be simplified a bit, but it's OK if not.

seberg · 2023-03-08T16:07:42Z

+            ('float16', '>e'),
+            ('float32', '>f'),
+            ('float64', '>d'),
+            # ('<U', '>U'),     # these get cast to '<U1' and '>U1'


Suggested change

# ('<U', '>U'), # these get cast to '<U1' and '>U1'

I think it is clear enough that U without a number is weird (I would love to just reject it one day for np.dtype(), but not for many dtype= calls where it has a clearer meaning).

seberg · 2023-03-08T17:19:54Z

+            non_native = non_native.replace('>', '<')
+            native = native.replace('<', '>')
+        non_native_dtype = np.dtype(non_native)
+        native_dtype = np.dtype(native)


You could probably simplify all of this by just starting with only the native above, and then use:

dtype.newbyteorder()

which swaps it by default.

seberg · 2023-03-13T08:03:14Z

+        if non_native_dtype.itemsize > 1:
+            # if the type is >1 byte, the non-native endian version
+            # must show endianness.
+            assert non_native_repr != native_repr,\


I much prefer avoiding \ if possible. In this case, I personally wouldn't care about the assert text anyway (pytest failures tend to print out the code anyway).

seberg

Just going to remove that "U1" logic as well, thanks for the follow up.

seberg · 2023-03-13T18:26:04Z

+        assert ('dtype' in native_repr) ^ (native_dtype in _typelessdata),\
+                ("an array's repr should show dtype if and only if the type "
+                 'of the array is NOT one of the standard types '
+                 '(e.g., int32, bool, float64).')


I would prefer this as an if native_dtype in [np.int_, np.float64, np.complex128, np.bool_] (or _tyeplessdata), but that is just me nitpicking (xor is much harder to reason about).

If you'd prefer something like

msg = ("an array's repr should show dtype if and only if the type " 'of the array is NOT one of the standard types ' '(e.g., int32, bool, float64).') if native_dtype in _typelessdata: assert 'dtype' in native_repr, msg else: assert 'dtype' not in native_repr, msg

I see nothing wrong with that. That doesn't seem much more clear to me, but maybe I'm the only one.

To me it is a bit nicer, especially if the msg was a comment on the branch. But we already cleaned up the test a lot and this doesn't matter enough.

Thanks @molsonkiko it is a nice improvement/fix.

to avoid change in printed dtype caused by numpy/numpy#23295

show endianness when nonstandard: address #19059

718a6f1

Try to fix problem where, for example, np.array([1], dtype='>u2') and np.array([1], dtype='<u2') both got represented as np.array([1], dtype=uint16)

seberg reviewed Feb 28, 2023

View reviewed changes

see if dtype.isnative works to show endianness

0d3bc7f

@seberg pointed out that dtype.isnative could be used in place of my previous composite check. Now all tests in core/tests/test_arrayprint.py pass (except the one xfail)

charris added the 01 - Enhancement label Mar 3, 2023

charris changed the title ~~show endianness when nonstandard: address #19059~~ ENH: show endianness when nonstandard: address #19059 Mar 3, 2023

molsonkiko added 4 commits March 3, 2023 17:02

reformat code with shed

eeaa2a9

Revert "reformat code with shed"

ae7b9c0

This reverts commit eeaa2a9. I wanted to fix the linter errors, but reformatting with shed didn't help, so I'll just do it manually.

fix lint bugs, use pytest.mark.parameterize

6e3f94c

fig bug in test_arrayprint, remove extra line

de79faa

eric-wieser reviewed Mar 4, 2023

View reviewed changes

tylerjereddy reviewed Mar 4, 2023

View reviewed changes

molsonkiko added 2 commits March 4, 2023 16:33

improve test coverage and variable naming

4d7b6dc

change _construction_repr(dtype) to str(dtype)

3e3d43e

mattip closed this Mar 12, 2023

mattip reopened this Mar 12, 2023

seberg reviewed Mar 13, 2023

View reviewed changes

simplify tests

53adf79

seberg approved these changes Mar 13, 2023

View reviewed changes

Apply suggestions from code review

7e2ef9b

seberg changed the title ~~ENH: show endianness when nonstandard: address #19059~~ ENH: show dtype in array repr when endianness is non-native Mar 14, 2023

seberg merged commit 071388f into numpy:main Mar 14, 2023

This was referenced Mar 14, 2023

Confusing ndarray repr: always shows dtype as if in native byteorder #19059

Closed

core.arrayprint.dtype_short_repr does not print byte order #11102

Closed

molsonkiko deleted the dtype-endian-repr branch March 14, 2023 14:20

saimn mentioned this pull request Mar 17, 2023

TST: dtype doctest failures in FITS examples astropy/astropy#14545

Closed

pllim added a commit to pllim/astropy that referenced this pull request Mar 17, 2023

TST: Only run some doctest with numpy<1.25

af9c049

to avoid change in printed dtype caused by numpy/numpy#23295

pllim mentioned this pull request Mar 17, 2023

TST: Only run some doctest with numpy<1.25 astropy/astropy#14550

Merged

pllim added a commit to pllim/astropy that referenced this pull request Mar 17, 2023

TST: Only run some doctest with numpy<1.25

e22607f

to avoid change in printed dtype caused by numpy/numpy#23295

cmarmo pushed a commit to cmarmo/astropy that referenced this pull request Mar 24, 2023

TST: Only run some doctest with numpy<1.25

1667c56

to avoid change in printed dtype caused by numpy/numpy#23295

mroeschke mentioned this pull request Jul 17, 2024

BUG: >M8[ns] series from >M8[ns] numpy array fails on little endian system and vice versa pandas-dev/pandas#51041

Closed

2 of 3 tasks

Conversation

molsonkiko commented Feb 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerjereddy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molsonkiko commented Mar 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattip commented Mar 12, 2023

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eric-wieser Mar 4, 2023 •

edited

Loading

molsonkiko commented Mar 10, 2023 •

edited

Loading