Skip to content
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: Changed repr of np.bool_ #17592

Closed
wants to merge 9 commits into from

Conversation

ganesh-k13
Copy link
Member

Changes:

Changed repr of np.bool_

resolves #12950
refers: #5942, #7913

After Fix:

>>> str(np.bool_(False))
'numpy.False_'
>>> str(np.bool_(True))
'numpy.True_'
>>> 

Note:

I expect a lot of UT and Doc failures, will fix as we go along.

cc: @eric-wieser , @seberg

@ganesh-k13
Copy link
Member Author

Some tests are forming cores, will send the mail to the mailing list, once that's fixed

@ganesh-k13
Copy link
Member Author

Do macs have a different set of tests? I noticed this is failing.

@ganesh-k13
Copy link
Member Author

It's working in linux:

(Pdb) p numpy_f
<function any at 0x7f78f15e7310>
(Pdb) p numpy_f(a, axis=1, keepdims=True)[...,:-1]
masked_array(
  data=[[[True, False, True]],

        [[True, False, True]]],
  mask=[[[False, False, False]],

        [[False, False, False]]],
  fill_value=numpy.True_)
> /home/ganesh/open-source/numpy/build/testenv/lib/python3.8/site-packages/numpy/ma/tests/test_core.py(4948)testkeepdims()
-> assert_equal(ma_f(a, keepdims=True).shape,
(Pdb) p ma_f(a, axis=1, keepdims=True)[...,:-1]
masked_array(
  data=[[[True, False, True]],

        [[True, False, True]]],
  mask=[[[False, False, False]],

        [[False, False, False]]],
  fill_value=numpy.True_)
(Pdb) p f
'any'

for f in ['sum', 'prod', 'mean', 'var', 'std']:
testaxis(f, a, d)
testkeepdims(f, a, d)

Any idea why it's failing only on Mac?

@eric-wieser
Copy link
Member

The test that is failing is a documentation test, and I guess the documentation tests are only run on mac? You need to update the docstrings to use the new printing.

@mattip
Copy link
Member

mattip commented Oct 21, 2020

The incantation is python runtests.py -g --refguide-check

@ganesh-k13 ganesh-k13 force-pushed the enh_12950_bool_repr branch 2 times, most recently from 4dd12ce to 9ae76ee Compare October 23, 2020 14:22
@ganesh-k13
Copy link
Member Author

ganesh-k13 commented Oct 24, 2020

I have emailed the mailing list (nabble link, pipermail link)for their opinion @eric-wieser, as mentioned in the issue.

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add an "improvements" release note, like we did when we improved void.__repr__ with https://numpy.org/doc/stable/release/1.14.0-notes.html#void-datatype-elements-are-now-printed-in-hex-notation?

ganesh-k13 added a commit to ganesh-k13/numpy that referenced this pull request Oct 26, 2020
ganesh-k13 added a commit to ganesh-k13/numpy that referenced this pull request Oct 30, 2020
@ganesh-k13
Copy link
Member Author

@eric-wieser , any more changes needed?

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and the mailing list seems not to care. Will leave this for another maintainer to push the button on if they agree.

I reworded the release note slightly to make clear also what did not change.

@ganesh-k13
Copy link
Member Author

Thanks Eric, sure

@seberg
Copy link
Member

seberg commented Nov 2, 2020

Happy to put this in as such, although I wonder a bit whether this has the potential to be a slightly noisy transition, and 1.20 is already a big release.

@mattip mattip added the triage review Issue/PR to be discussed at the next triage meeting label Nov 4, 2020
@hameerabbasi
Copy link
Contributor

This is different from every other NumPy scalar, np.int*, np.float*, ...

Also reprs don't usually include the module.

@seberg
Copy link
Member

seberg commented Nov 4, 2020

We are a bit unsure whether this is a good idea at all, could you propose this on the mailing list, since it is an API change?

@eric-wieser
Copy link
Member

This already hit the mailing list and was meet with silence

@mattip
Copy link
Member

mattip commented Apr 18, 2022

Summarizing the current status:

  • this PR should be retitled to reflect the pivot to changing the repr of all scalars
  • tests are failing and it is still marked as a draft
  • @eric-wieser could you remove your approval of the PR, it is confusing when looking at PR statuses.

seberg pushed a commit to seberg/numpy that referenced this pull request Sep 19, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Sep 19, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 14, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 14, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 14, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 14, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 24, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Oct 24, 2022
seberg pushed a commit to seberg/numpy that referenced this pull request Jun 21, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jun 21, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jun 30, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jun 30, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jul 6, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jul 6, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jul 18, 2023
seberg pushed a commit to seberg/numpy that referenced this pull request Jul 18, 2023
@seberg
Copy link
Member

seberg commented Jul 26, 2023

The changes here were included in gh-22449 so closing this one.

@seberg seberg closed this Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change __repr__ of np.bool_
6 participants