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

DOC: Clarify isreal docstring #18843

Merged
merged 16 commits into from
May 7, 2021
Merged

Conversation

amrit110
Copy link
Contributor

@amrit110 amrit110 commented Apr 23, 2021

Add a clarification to docstring of isreal method. Raised in #18814, isreal returns True for non-numeric inputs like None.

Closes #18814

Add clarification to docstring of isreal function,
that it returns True for non-numeric inputs.
Closes numpy#18814.
numpy/lib/type_check.py Outdated Show resolved Hide resolved
Add clarification to Notes section
Add clarification also as markup to serve as warning in docstring
Add clarification to Notes section
Add clarification also as markup to serve as warning in docstring
@charris
Copy link
Member

charris commented Apr 24, 2021

You have a long line that needs to be < 80 characters.

@amrit110
Copy link
Contributor Author

You have a long line that needs to be < 80 characters.

@charris thanks for pointing out, fixed!

numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
@charris charris changed the title Add clarification to docstring for isreal DOC: Clarify isreal docstring Apr 24, 2021
Comment on lines 285 to 286
>>> np.isreal([None, False, "imaginary", complex])
array([True, True, True, True])
Copy link

@cuspymd cuspymd Apr 27, 2021

Choose a reason for hiding this comment

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

When tested, it returns False instead of an array in the following cases.

>>> np.isreal([2j, "a"])
/.../lib/python3.6/site-packages/numpy/lib/type_check.py:277: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison
  return imag(x) == 0
False

Copy link
Member

Choose a reason for hiding this comment

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

That happens for string arrays, on current master, it should run into two future warnings, because the original input is already a mix of string and complex... Maybe fine to ignore.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep the Notes section as it is, and move the .. note:: directive down there with more details that the function does not work for string arrays, and for dtype=object currently always returns True even if the content is complex.

If we want an example, maybe include np.array([1+2j, ...], dtype=object) (a complex number and the dtype=object explicitly)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cuspymd, @seberg, thanks for the comments. i've made fixes by moving the .. note:: directive down to the examples section as well as including the example for string array and mentioning that the function doesn't work, as well as the array with python objects returning all True even if it consists complex numbers. let me know if more clarifications can be added!

numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
numpy/lib/type_check.py Outdated Show resolved Hide resolved
amrit110 and others added 4 commits May 6, 2021 12:25
fix to docstring

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
improve note to explicitly warn about string or object arrays

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
remove note directive in examples section

Co-authored-by: Ross Barnowski <rossbar@berkeley.edu>
numpy/lib/type_check.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for sticking with all the iterations @amrit110 !

@rossbar
Copy link
Contributor

rossbar commented May 7, 2021

CI failures are unrelated - the doctests in the circleci job are green and the rendered page looks good.

@amrit110
Copy link
Contributor Author

amrit110 commented May 7, 2021

thanks @rossbar ! can i merge it or is to be done by one of the maintainers?

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.

np.isreal(None) is True
6 participants