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

Allow numbers.Integral instead of only built-in int for QuantumRegister index #4591

Merged
merged 25 commits into from
Jul 22, 2020

Conversation

arunraja-hub
Copy link
Contributor

@arunraja-hub arunraja-hub commented Jun 18, 2020

Summary

Fixes #3929

Details and comments

-I changed line 120/121 of register.py such that it includes numbers.Integral as an allowable type for an index of a QuantumRegister. This is my first issue! Excited to contribute more
Issue: #3929

@arunraja-hub arunraja-hub requested a review from a team as a code owner June 18, 2020 17:36
@CLAassistant
Copy link

CLAassistant commented Jun 18, 2020

CLA assistant check
All committers have signed the CLA.

@ajavadia ajavadia changed the title fixed issue 3929 allow using np.int for QuantumRegister size Jun 19, 2020
@Cryoris
Copy link
Contributor

Cryoris commented Jun 19, 2020

Thanks for the contribution @arunraja-hub! Can you add a test that checks passing different types of integers as register sizes? I.e. a test that fails without your PR but works afterwards.
And could you also add a bugfix releasenote, where you're saying that not only Python int are allowed but any integer of type numbers.Integral? How add a releasenote is described here.

@Cryoris Cryoris added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Jun 19, 2020
@arunraja-hub
Copy link
Contributor Author

arunraja-hub commented Jun 20, 2020

Hi @Cryoris , Thank you! Could you provide more info on implementing the test? is there any documentation on that?

@Cryoris Cryoris changed the title allow using np.int for QuantumRegister size Allow numbers.Integral instead of only built-in int for QuantumRegister size Jun 22, 2020
Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

Here are some comments on the contribution 🙂

@Cryoris
Copy link
Contributor

Cryoris commented Jun 22, 2020

I don't think there's a dedicated documentation for the tests, but essentially a test is a short snippet of code that checks your new feature works. In a bugfix test, you ideally have a snippet that didn't work before but now does. E.g. something like

def test_register_int_types(self):
    ints = [int(2), int64(2), np.int(2), np.integer(2), ... ] # all int types you can think of
    for size in ints:
        with self.subTest(size=size):  # technically not necessary, but nicer for debugging if you add this
            qr = QuantumRegister(size)
            cr = ClassicalRegister(size)
            self.assertEqual(qr.size, 2)
            self.assertEqual(cr.size, 2)

See test/python/circuit/test_circuit_registers.py for where you could implement the test and how others look like.

Let me know if you have any questions!

@arunraja-hub
Copy link
Contributor Author

arunraja-hub commented Jun 23, 2020

test_register_int_types passed when I ran the command make test
However, according to github's checks below there seem to be 2 cases that are failing. Why is this the case?

@Cryoris
Copy link
Contributor

Cryoris commented Jun 24, 2020

The CI also runs a linter and stylechecker on the code, you can do this locally by doing make lint and make style in the command line. The output is

test/python/circuit/test_circuit_registers.py:61:1: W293 blank line contains whitespace
test/python/circuit/test_circuit_registers.py:63:77: W291 trailing whitespace
test/python/circuit/test_circuit_registers.py:65:44: E202 whitespace before ']'
test/python/circuit/test_circuit_registers.py:65:46: W291 trailing whitespace
test/python/circuit/test_circuit_registers.py:67:42: W291 trailing whitespace

If you fix this, the tests will passs.

@arunraja-hub
Copy link
Contributor Author

arunraja-hub commented Jun 25, 2020

@Cryoris Completed the tests for int32 and int64. I believe this PR is all set
Do I need to do anything else?

@Cryoris Cryoris changed the title Allow numbers.Integral instead of only built-in int for QuantumRegister size Allow numbers.Integral instead of only built-in int for QuantumRegister index Jun 30, 2020
@Cryoris
Copy link
Contributor

Cryoris commented Jun 30, 2020

Actually I misunderstood the goal, I though the size should accept all types not the index. The tests should be changed accordingly to check that all these integer types work as indices of QuantumRegister and ClassicalRegister. You could e.g. change the test to do something like

for index in [np.int(2), np.int32(2), ... ]:
        # ... sub test as before, and the same for classical register
        qr = QuantumRegister(3)
        self.assertEqual(qr[index], qr[2])

@arunraja-hub
Copy link
Contributor Author

Ths issue was that I had a newline. So I removed. Now the lint error is that I have removed a newline. So it is contradictory
https://dev.azure.com/qiskit-ci/qiskit-terra/_build/results?buildId=16523&view=logs&j=7684b0c9-af38-5400-184f-6880bd8a4dd7&t=9d95188b-4e59-58d8-f7c4-3f1fbd4209ad&l=12

qiskit/circuit/register.py Outdated Show resolved Hide resolved
Co-authored-by: Kevin Krsulich <kevin@krsulich.net>
@arunraja-hub arunraja-hub requested a review from kdk July 21, 2020 18:35
qiskit/circuit/register.py Outdated Show resolved Hide resolved
@arunraja-hub
Copy link
Contributor Author

Hi @Cryoris, May I confirm that this PR is complete? Also, could you assign this issue to me? Thank you

@Cryoris
Copy link
Contributor

Cryoris commented Jul 22, 2020

Yes, it is! Thanks a lot for the contribution 🚀

@arunraja-hub
Copy link
Contributor Author

Thank you @Cryoris
Are there other issues I can work on next?

@arunraja-hub arunraja-hub reopened this Jul 22, 2020
@mergify mergify bot merged commit ec5e537 into Qiskit:master Jul 22, 2020
garrison added a commit to garrison/qiskit that referenced this pull request Mar 28, 2022
I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by Qiskit#4591.
mergify bot added a commit that referenced this pull request Apr 13, 2022
* Allow `shots` to be a `numpy.int64`

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by #4591.

* Allow shots to be either an int or a numpy.integer, explicitly

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mergify bot pushed a commit that referenced this pull request Apr 13, 2022
* Allow `shots` to be a `numpy.int64`

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by #4591.

* Allow shots to be either an int or a numpy.integer, explicitly

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d9a6f0b)
mergify bot added a commit that referenced this pull request Apr 13, 2022
* Allow `shots` to be a `numpy.int64`

I've noticed that the Aer backends, all work properly if the `shots`
count is passed as a `numpy.int64`, but the BasicAer and hardware
backends both fail in this case.  The simplest way to make it work
on all backends is by making this change in terra, inspired by #4591.

* Allow shots to be either an int or a numpy.integer, explicitly

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit d9a6f0b)

Co-authored-by: Jim Garrison <garrison@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a numpy integer type as an index for a QuantumRegister fails
4 participants