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

EHN: make real_if_close elementwise #11375

Open
homocomputeris opened this issue Jun 18, 2018 · 10 comments
Open

EHN: make real_if_close elementwise #11375

homocomputeris opened this issue Jun 18, 2018 · 10 comments

Comments

@homocomputeris
Copy link

homocomputeris commented Jun 18, 2018

@seberg has pointed out that this function in not elementwise (yeah, I can't read the docs, obviously). It would be nice to have the possibility to apply it to each element.

import numpy as np

a = -0. - 2.03541e-16j
v = np.array([a, a, a, a])
M = np.array([[a, 0. + 1.00000e+00j, a],
              [-0. - 1.29099e+00j, 0. + 0.00000e+00j, 0. + 1.29099e+00j],
              [0. + 3.33333e+00j, -0. - 6.66667e+00j, 0. + 3.33333e+00j]])

print(np.real_if_close(a))
print(np.real_if_close(v))
print(np.real_if_close(M))

gives

-0.0
[-0. -0. -0. -0.]
[[-0.-2.03541e-16j  0.+1.00000e+00j -0.-2.03541e-16j]
 [-0.-1.29099e+00j  0.+0.00000e+00j  0.+1.29099e+00j]
 [ 0.+3.33333e+00j -0.-6.66667e+00j  0.+3.33333e+00j]]

and M is not changed.

Version info:

'1.14.3'
blas_mkl_info:
    libraries = ['mkl_rt', 'pthread']
    library_dirs = ['/opt/intel/mkl/lib/intel64/']
    define_macros = [('SCIPY_MKL_H', None), ('HAVE_CBLAS', None)]
    include_dirs = ['/opt/intel/composerxe/linux/mkl', '/opt/intel/composerxe/linux/mkl/include', '/opt/intel/composerxe/linux/mkl/lib', '/opt/intel/mkl/include']
blas_opt_info:
    libraries = ['mkl_rt', 'pthread']
    library_dirs = ['/opt/intel/mkl/lib/intel64/']
    define_macros = [('SCIPY_MKL_H', None), ('HAVE_CBLAS', None)]
    include_dirs = ['/opt/intel/composerxe/linux/mkl', '/opt/intel/composerxe/linux/mkl/include', '/opt/intel/composerxe/linux/mkl/lib', '/opt/intel/mkl/include']
lapack_mkl_info:
    libraries = ['mkl_rt', 'pthread']
    library_dirs = ['/opt/intel/mkl/lib/intel64/']
    define_macros = [('SCIPY_MKL_H', None), ('HAVE_CBLAS', None)]
    include_dirs = ['/opt/intel/composerxe/linux/mkl', '/opt/intel/composerxe/linux/mkl/include', '/opt/intel/composerxe/linux/mkl/lib', '/opt/intel/mkl/include']
lapack_opt_info:
    libraries = ['mkl_rt', 'pthread']
    library_dirs = ['/opt/intel/mkl/lib/intel64/']
    define_macros = [('SCIPY_MKL_H', None), ('HAVE_CBLAS', None)]
    include_dirs = ['/opt/intel/composerxe/linux/mkl', '/opt/intel/composerxe/linux/mkl/include', '/opt/intel/composerxe/linux/mkl/lib', '/opt/intel/mkl/include']
@seberg
Copy link
Member

seberg commented Jun 18, 2018

The function may be weird, but it does exactly what the documentation says it does? M is not almost real, so it is not made a real array. Note the function does not say "round to real if close and if all is real give it a real dtype".

@homocomputeris
Copy link
Author

True. I thought it was elementwise, like abs, isnan etc. This would make more sense

@homocomputeris homocomputeris changed the title BUG: real_if_close does not affect values in some arrays EHN: make real_if_close elementwise Jun 18, 2018
@homocomputeris
Copy link
Author

Well, the question is why it's not elementwise? I can't imagine any usecase for current behaviour.

@seberg
Copy link
Member

seberg commented Jun 18, 2018

The use case is probably to call it on an input that requires a real datatype, which most of the time would probably just error out if the input was still complex then. Not that I really know that the use case is very useful, heh.

@seberg
Copy link
Member

seberg commented Jun 18, 2018

Also:

  1. Note that you cannot make it elementwise and retain the casting behaviour obviously. You would have a "round imaginary part to zero if it is close to zero" function
  2. These functions are old and frankly some of them may have a bit narrow use case, or a use case which was made for quick scripting or input sanitizing and not quite as much for consistency as you may be used to.

@seberg
Copy link
Member

seberg commented Jun 18, 2018

Anyway, I doubt we should change the function, we could add a kwarg argument, or since I doubt it is used much, create a new similar one and deprecate this one (but should ping the list or check downstream first, it is not unlikely that I am completely misjudging and this function is used a lot).

@eric-wieser
Copy link
Member

I'm not sure this function is useful enough to be in numpy in the first place, let alone having two similar buyt different copies - For what @homocomputeris wants, np.where(np.isclose(x.imag, 0), x.real, x) does the job

@eric-wieser
Copy link
Member

Scipy uses real_if_close exactly once

@homocomputeris
Copy link
Author

@eric-wieser It seems that where is more universal and more pythonic.
If the function is rarely used and not very useful, it might be worth deprecating it.

@eendebakpt
Copy link
Contributor

@eric-wieser @rgommers The real_if_close is mentioned in #23537 as a possible candidate for deprecation. In my own code is make use of the method for at least three use-cases for the method. For 2 of them there are in fact better alternatives

  • Eigenvalues of real symmetric matrix or complex conjugate matrix are real, but can have very small
    imaginary component when calculated with np.linalg.eig due to floating point precision. Using real_if_close these are cast to real. If beforehand it is already known the matrix is symmetric, then instead np.linalg.eigh can be used.

  • The result of X * X.conjugate() is real, but can have small imaginary coefficients. Instead of np.real_if_close(X * X.conjugate()) use np.abs(X)**2

  • When displaying complex vectors, or in my case: quantum mechanical state vectors in braket notation it is much nicer
    to display |0> + |1> instead of (1+0j)|0> + (1+0j)|1> or (1+1e-16j)|0> + |1>.
    Here real_if_close is convenient to apply to the individual coefficients. (note: the small imaginary coefficients appear when performing multiple rotations of exact angles like pi or pi/2)

In qiskit the real_if_close is used: https://github.com/search?q=repo%3AQiskit%2Fqiskit-terra%20real_if_close&type=code
(reasons similar to the first two items)

Because of the last item I would prefer the real_if_close to remain available (but I have no objections to moving it out of the main namespace)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants