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

Remove DeprecationWarning of np.int, np.bool, np.complex #5758

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented Feb 1, 2021

Summary

Resolves: #5756

Numpy 1.20.0 deprecates Using the aliases of builtin types like np.int.
See the release note.

@ikkoham ikkoham changed the title Fix deprecation of np.int, np.bool, np.complex Remove DeprecationWarning of np.int, np.bool, np.complex Feb 1, 2021
@@ -131,7 +131,7 @@ def __getitem__(self, key):
CircuitError: if the `key` is not an integer.
QiskitIndexError: if the `key` is not in the range `(0, self.size)`.
"""
if not isinstance(key, (int, np.int, np.int32, np.int64, slice, list)):
if not isinstance(key, (int, np.int32, np.int64, slice, list)):
Copy link
Member

@t-imamichi t-imamichi Feb 1, 2021

Choose a reason for hiding this comment

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

The following might be simpler. If we want reject np.int16 etc., the current code is appropriate.

Suggested change
if not isinstance(key, (int, np.int32, np.int64, slice, list)):
if not isinstance(key, (int, np.integer, slice, list)):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why it does not accept np.int8 and np.int16, if the code owner is ok, please commit it.

Copy link
Member

Choose a reason for hiding this comment

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

The original code was committed as part of #4810. if not isinstance(key, (int, np.integer, slice, list)) looks more appropriate to me. @kdk What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is probably fine, #4810 was a response to the performance regression in #4791 and just selected the types that were in the original issue about numpy types not working. np.integer shouldn't have the overhead that numbers.Integral introduced and would provide better matching for short types so I think it is a better solution. @t-imamichi do you want to push a follow up PR switching the numpy type checking to this instead?

@@ -290,7 +290,7 @@ def __len__(self):
def __getitem__(self, qubits):
"""Return the unsigned Pauli group Pauli for subset of qubits."""
# Set group phase to 0 so returned Pauli is always +1 coeff
if isinstance(qubits, (int, np.int)):
if isinstance(qubits, int):
Copy link
Member

Choose a reason for hiding this comment

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

If qubits can be a numpy integer, we need the following.

Suggested change
if isinstance(qubits, int):
if isinstance(qubits, (int, np.integer)):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure. If the code owner is ok, please commit it.

Copy link
Member

Choose a reason for hiding this comment

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

@chriseclectic Which did you intend, isinstance(qubits, int) or isinstance(qubits, (int, np.integer))?

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
@@ -316,7 +316,7 @@ def delete(self, qubits):
QiskitError: if ind is out of bounds for the array size or
number of qubits.
"""
if isinstance(qubits, (int, np.int)):
if isinstance(qubits, int):
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Suggested change
if isinstance(qubits, int):
if isinstance(qubits, (int, np.integer)):

if isinstance(qubits, (int, np.int)):
ret = Pauli((np.zeros(ret_qubits, dtype=bool),
np.zeros(ret_qubits, dtype=bool)))
if isinstance(qubits, int):
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Suggested change
if isinstance(qubits, int):
if isinstance(qubits, (int, np.integer)):

@@ -242,7 +242,7 @@ def __getitem__(self, key):
"""Return a view of the PauliTable."""
# Returns a view of specified rows of the PauliTable
# This supports all slicing operations the underlying array supports.
if isinstance(key, (int, np.int)):
if isinstance(key, int):
Copy link
Member

@t-imamichi t-imamichi Feb 1, 2021

Choose a reason for hiding this comment

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

same as above

Suggested change
if isinstance(key, int):
if isinstance(key, (int, np.integer)):

@@ -128,7 +128,7 @@ def __getitem__(self, key):
"""Return a view of the SparsePauliOp."""
# Returns a view of specified rows of the PauliTable
# This supports all slicing operations the underlying array supports.
if isinstance(key, (int, np.int)):
if isinstance(key, int):
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Suggested change
if isinstance(key, int):
if isinstance(key, (int, np.integer)):

@ikkoham
Copy link
Contributor Author

ikkoham commented Feb 1, 2021

Since this is a blocker for the whole Qiskit, I think it's best to merge it ASAP. So I'm trying to keep the behavior unchanged. (Note that np.int is an alias of int.) In particular, the quantum_info module might be better changed to np.bool_ and np.int64 for performance reasons, but that should be discussed in another issue.

@t-imamichi
Copy link
Member

Yes, it makes sense to discuss the type of isinstance in another place. I leave my comments for the future discussion, but, I am OK to merge this PR as it is.

Copy link
Member

@1ucian0 1ucian0 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the quick fix!

@1ucian0 1ucian0 merged commit 5f9da19 into Qiskit:master Feb 1, 2021
@mtreinish mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Feb 5, 2021
mergify bot pushed a commit that referenced this pull request Feb 5, 2021
* fix deprecation of np.int, np.bool, np.complex

* Update qiskit/opflow/primitive_ops/pauli_op.py

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>

Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
(cherry picked from commit 5f9da19)

# Conflicts:
#	qiskit/opflow/primitive_ops/pauli_op.py
#	qiskit/opflow/primitive_ops/pauli_sum_op.py
#	qiskit/quantum_info/operators/measures.py
#	qiskit/quantum_info/operators/symplectic/base_pauli.py
#	qiskit/quantum_info/operators/symplectic/clifford.py
#	qiskit/quantum_info/operators/symplectic/pauli.py
#	qiskit/quantum_info/operators/symplectic/random.py
#	qiskit/visualization/pulse_v2/generators/waveform.py
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Feb 5, 2021
The latest release of numpy, 1.20.0, deprecated type aliases for
python's built in numeric types (complex, int, float, etc). The usage of
these aliases has caused a myriad of deprecation warnings on import and
during runtime everytime they were used. This commit fixes this so that
terra's 0.16.x series is compatible with numpy 1.20.0.

This commit is effectively a backport of Qiskit#5758 and Qiskit#5768 but because of
significant divergance between master and stable/0.16 it was not a
straight backport.
mergify bot pushed a commit that referenced this pull request Feb 5, 2021
* Fix compatibility with numpy 1.20.0

The latest release of numpy, 1.20.0, deprecated type aliases for
python's built in numeric types (complex, int, float, etc). The usage of
these aliases has caused a myriad of deprecation warnings on import and
during runtime everytime they were used. This commit fixes this so that
terra's 0.16.x series is compatible with numpy 1.20.0.

This commit is effectively a backport of #5758 and #5768 but because of
significant divergance between master and stable/0.16 it was not a
straight backport.

* Disable pylint rule causing failure with fastjsonschema 2.15.0 (#5767)

* Disable pylint rule causing failure with fastjsonschema 2.15.0

On Feb. 1, 2021 fastjsonschema 2.15.0 was released that made some
changes to the exception API. [1][2] These changed the inheritance tree
used for the exception classes so that 'JsonSchemaException' (which is
the class qiskit is catching) was changed to be a base class instead of
the class raised by validation errors. A new class
'JsonSchemaValueException' a subclass of that now base class is now raised
by validation errors. The attributes used to wrap that exception and
raise a custom exception are now defined in `JsonSchemaValueException`
and not 'JsonSchemaException'. This is causing pylint to fail because
the attributes are not defined in the base class. But in practice this
will not raise an error as `JsonSchemaValueException` will always be
raised even if we only isinstance check its base class. To still support
old versions of fastjsonschema we shouldn't change that isinstance check
so this commit just disables the rules. Especially since the qobj
jsonschema validation is deprecated and will be removed in a future
release as long as the code works pylint's pedantry isn't as important.

[1] horejsek/python-fastjsonschema@64a635e
[2] https://github.com/horejsek/python-fastjsonschema/blob/master/CHANGELOG.txt#L6-L8

* Fix lint in lint disable comment

(cherry picked from commit 56facba)

* Fix stray deprecated types
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Feb 15, 2021
In Qiskit#5617 a single usage of np.int slipped in to the clifford module.
This type is deprecated in numpy 1.20.0 and is just an alias for
python's stdlib int type (see Qiskit#5788 and Qiskit#5758 for the earlier fixes).
This usage is causing downstream failures in other project's CI that
use this module in environment's where warnings aren't allowed (mainly
ignis's docs builds). This commit fixes this stray np.int usage to
unblock CI and remove the deprecation warning from anything using the
clifford module.
mergify bot pushed a commit that referenced this pull request Feb 15, 2021
In #5617 a single usage of np.int slipped in to the clifford module.
This type is deprecated in numpy 1.20.0 and is just an alias for
python's stdlib int type (see #5788 and #5758 for the earlier fixes).
This usage is causing downstream failures in other project's CI that
use this module in environment's where warnings aren't allowed (mainly
ignis's docs builds). This commit fixes this stray np.int usage to
unblock CI and remove the deprecation warning from anything using the
clifford module.

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
@kdk kdk added the Changelog: None Do not include in changelog label Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Many lines of DeprecationWarning (numpy 1.20.0)
5 participants