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.

Sign up for GitHub

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

Jump to bottom

Fix an isintanceof statement to allow all numpy integers #5768

Merged
merged 5 commits into from
Feb 15, 2021
Merged

Fix an isintanceof statement to allow all numpy integers #5768

merged 5 commits into from
Feb 15, 2021

Conversation

t-imamichi
Copy link
Member

@t-imamichi t-imamichi commented Feb 2, 2021 •
edited by mtreinish
Loading

Summary

Fix an isinstance of statement to allow all numpy integers.

Discussed in #5758

Details and comments

@t-imamichi t-imamichi requested a review from a team as a code owner February 2, 2021 06:08
@mtreinish
Copy link
Member

This LGTM, but I think we should also do this in the quantum_info module too as you pointed out in #5758. I'm pretty sure the use of np.int there before was a mistake and the intent was to be np.integer to also allow numpy integer types.

@t-imamichi
Copy link
Member Author

I'm wondering it because np.int is not always used. For example, PauliTable.__getitem__ used np.int, but delete did not as follows.
https://github.com/Qiskit/qiskit-terra/blob/0bb570f4a3ecbd8f830866f7ca1306fe2e028c9c/qiskit/quantum_info/operators/symplectic/pauli_table.py#L245
https://github.com/Qiskit/qiskit-terra/blob/0bb570f4a3ecbd8f830866f7ca1306fe2e028c9c/qiskit/quantum_info/operators/symplectic/pauli_table.py#L274
StabilizerTables did not use np.int either.
https://github.com/Qiskit/qiskit-terra/blob/0bb570f4a3ecbd8f830866f7ca1306fe2e028c9c/qiskit/quantum_info/operators/symplectic/stabilizer_table.py#L256
Note that they are files of the stable branch.

It's good to use the same type of index (just int or (int, np.integer)) among all symplectic objects, at least within an object (pauli_table).
Which type of index should I use?

@chriseclectic
Copy link
Member

chriseclectic commented Feb 4, 2021 •
edited
Loading

The intent of most of these locations in qiskit was to allow python or numpy integer types. There were occasional cases for some functions/classes where a numpy int was used if it came from something like index = array[i]. In those cases index would have type np.int64 which would return false for isinstance(x, int)).

@t-imamichi
Copy link
Member Author

@chriseclectic Thank you for your information. I replace isinstance(int) in symplectic with isinstance(int, np.integer).

@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
@mtreinish mtreinish mentioned this pull request Feb 5, 2021
Closed
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Feb 5, 2021
@mtreinish
Fix compatibility with numpy 1.20.0
517e07d 
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.
@mtreinish mtreinish mentioned this pull request Feb 5, 2021
Merged
@mtreinish mtreinish removed 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
@mtreinish
Fix compatibility with numpy 1.20.0 (#5788)
399a51e 
* 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
@t-imamichi
Copy link
Member Author

t-imamichi commented Feb 15, 2021 •
edited
Loading

I replaced all isinstance(*, int) with isinstance(*, (int, np.integer)) in quantum_info.operators. Could you verify my changes?

$ egrep 'isinstance.*int' qiskit/quantum_info/operators/**/*.py
qiskit/quantum_info/operators/symplectic/pauli.py:        if isinstance(qubits, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/pauli.py:        if isinstance(qubits, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/pauli.py:        if isinstance(qubits, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/pauli_table.py:        if isinstance(key, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/pauli_table.py:        if isinstance(ind, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/pauli_table.py:        if not isinstance(ind, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py:        if isinstance(key, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/stabilizer_table.py:        if isinstance(key, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/stabilizer_table.py:        if isinstance(ind, (int, np.integer)):
qiskit/quantum_info/operators/symplectic/stabilizer_table.py:        if not isinstance(ind, (int, np.integer)):

mtreinish
mtreinish approved these changes Feb 15, 2021
View reviewed changes
Copy link
Member

@mtreinish mtreinish 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 updating.

@mergify mergify bot merged commit 00c0518 into Qiskit:master Feb 15, 2021
@t-imamichi t-imamichi deleted the fix-np_integer branch February 16, 2021 02:10
@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
Reviewers

@mtreinish mtreinish mtreinish approved these changes

@chriseclectic chriseclectic Awaiting requested review from chriseclectic

Assignees
No one assigned
Labels
Changelog: None Do not include in changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t-imamichi @mtreinish @chriseclectic @kdk