Add Uniform State Preparation

[Hirmay]

Summary

Closes #11735

Details and comments

[Hirmay]

Just an update I have created a draft pull request here for the original issue #11735

[Hirmay]

@woodsp-ibm I have put this as draft as you mentioned before. But I don't understand the errors in some of these checks. Can you please give some insight regarding this?

[woodsp-ibm]

One aspect is the formatting of the code, the style is checked via black you need to run that on your code locally - see https://github.com/Qiskit/qiskit/blob/main/CONTRIBUTING.md#style-and-lint

lint: commands[0]> black --check qiskit test tools examples setup.py
would reformat /home/vsts/work/1/s/qiskit/circuit/library/data_preparation/state_preparation.py
Oh no! 💥 💔 💥
1 file would be reformatted, 1257 files would be left unchanged.

The other is a cyclic import - I copied some of the error below. You have add this import from qiskit.visualization import circuit_drawer but I did not see anything in the added code using it and it would seem strange to import visualization logic here anyway. I would remove that and it most likely will eliminate the cyclic error in doing so.

  File "/home/vsts/work/1/s/qiskit/circuit/library/data_preparation/state_preparation.py", line 31, in <module>
    from qiskit.visualization import circuit_drawer
  File "/home/vsts/work/1/s/qiskit/visualization/__init__.py", line 259, in <module>
    from .circuit import circuit_drawer
  File "/home/vsts/work/1/s/qiskit/visualization/circuit/__init__.py", line 15, in <module>
    from .circuit_visualization import circuit_drawer
  File "/home/vsts/work/1/s/qiskit/visualization/circuit/circuit_visualization.py", line 39, in <module>
    from qiskit.circuit import ControlFlowOp, Measure
ImportError: cannot import name 'ControlFlowOp' from partially initialized module 'qiskit.circuit' (most likely due to a circular import) (/home/vsts/work/1/s/qiskit/circuit/__init__.py)

================================================================================
The above traceback was encountered during test discovery which imports all the found test modules in the specified test_path.

You will need to add unit test(s) too, its good to have already as it will show the code is working as intended.

[Hirmay]

Hi @woodsp-ibm, I have done the changes as you suggested and even tested it locally. Please have a look into this and suggest future steps.

Thank you!

Edit: Sorry Steve, the tests passed locally and I tried fixing some of these ones, but I am unable to understand the Coverage report check which is causing the errors in later tests.

[woodsp-ibm]

It seems a test, that is a general test for equivalence, is picking up on your new "Gate" and failing. If you look that test has a exclusion list

qiskit/test/python/circuit/test_gate_definitions.py

Line 259 in a7cc166

class TestGateEquivalenceEqual(QiskitTestCase):

that already excludes StatePreparation - maybe this new one needs to be added there.

Since its failing on num_qubits, I was looking at StatePreparation and how it handles it there. Could this new one be made more compatible with that - ie compute for M the smallest n (where I think n there is synonomous with num_qubits right?) and let one expand things out otherwise by specifying num_qubits explicitly. I get that one can require num_qubits to always be specified - just wondering if it was done more like StatePreparation whether that would be good.

[Hirmay]

Thank you, Steve, for your helpful response. So, if the StatePreparation is being excluded, then regardless of how they handle it, it won't matter, correct? Because there will be no TestGateEquivalenceEqual test case performed on that. Let me know if I'm not understanding it properly or missing something. Then should I just try running the tests with this gate also included and see what happens?
Though looking more into StatePreparation, they specify num_qubits as optional.

qiskit/qiskit/circuit/library/data_preparation/state_preparation.py

Line 45 in a7cc166

num_qubits: Optional[int] = None,

Then they just get num_qubits through the _get_num_qubits method.

qiskit/qiskit/circuit/library/data_preparation/state_preparation.py

Line 111 in a7cc166

num_qubits = self._get_num_qubits(num_qubits, params)

[woodsp-ibm]

If StatePreparation is excluded from that test it made sense to me that your new class probably should be too.

Yes StatePreparation has num_qubits as optional and does that computation you refer to. It seemed like this could be done the same if that seems reasonable - it would perhaps make them more consistent in that regard.

[Hirmay]

Okay, that makes sense. I will try to do that as well. Thank you so much, Steve, for all the help and support!

[woodsp-ibm]

If you look at StatePreparation this type of check raises an error rather than doing and assert. I think assert if not used in Qiskit rather it does some checks and raises and error in generaI. Here I would check and raise a ValueError rather than being an assert - my take is that would be more standard than the QiskitError that StatePreparation uses.

Your unit tests should check these error paths and make sure an error is raised when expected.

[Hirmay]

You're right, I have replaced assert with raise ValueError. I will look into unit tests too. Thanks, again!

[Hirmay]

I have added two test cases, hopefully everything is all well now. Thank you, Steve, for everything!

[Hirmay]

@woodsp-ibm I think the reason why it is still giving the same error in Coverage report test is it might be testing it without the updated TestGateEquivalenceEqual which excludes this class, whereas the old one doesn't. One solution is to just submit a PR with the updated TestGateEquivalenceEqual and once it is merged try this, but that may not be preferable with the reviewers.

[woodsp-ibm]

Its not failing the same way though

test.python.circuit.library.test_generalized_uniform_superposition_gate.TestGeneralizedUniformSuperposition.test_generalized_uniform_superposition_gate
-------------------------------------------------------------------------------------------------------------------------------------------------------

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):

      File "/home/runner/work/qiskit/qiskit/test/python/circuit/library/test_generalized_uniform_superposition_gate.py", line 38, in test_generalized_uniform_superposition_gate
    gate = Generalized_Uniform_Superposition_Gate(M, n)

      File "/home/runner/work/qiskit/qiskit/qiskit/circuit/library/data_preparation/state_preparation.py", line 484, in __init__
    super().__init__("USup", num_qubits, [])

      File "/home/runner/work/qiskit/qiskit/qiskit/circuit/gate.py", line 46, in __init__
    super().__init__(name, num_qubits, 0, params, label=label, duration=duration, unit=unit)

      File "/home/runner/work/qiskit/qiskit/qiskit/circuit/instruction.py", line 80, in __init__
    raise CircuitError("num_qubits and num_clbits must be integer.")

    qiskit.circuit.exceptions.CircuitError: 'num_qubits and num_clbits must be integer.'


test.python.circuit.library.test_generalized_uniform_superposition_gate.TestGeneralizedUniformSuperposition.test_incompatible_M

this is from your code. In StatePreparation the super.init call is made later once the number of qubits is computed. Maybe compare to that as I think its failing due the super init call now. Most likely you end up passing None now because its optional so it needs to be done once you compute the value like StatePreparation does

[Hirmay]

Yes, Steve, the error is different, but only slightly. The fundamental error is the same. I think the error coming from ddt is not there because of the decorator I added later (which didn't go under checks, since I was constantly updating the files 😅 ). I believe if the same is checked with the class being excluded, this error won't be produced, and this is the only primary error currently.

[woodsp-ibm]

I am not understanding why params is here. I know they are passed to the parent but this is not something that is used here right - your only params are M. In your test, where you are doing Generalized_Uniform_Superposition_Gate(M, n) the M there goes to params (the first parameter in the list here), n ends up in M (the 2nd position) and num_qubits is left as None.

Hence this call fails in the parent super().__init__("USup", num_qubits, []) as it passes None (as far as I can see).

My take would be to remove params from here, have M first here, not have a default value so you have to provide a value, and then num_qubits as you have it.

Next move this call super().__init__("USup", num_qubits, []) to the end of the method and change it to super().__init__("USup", self._num_qubits, [M]) This way you always pass a valid value of num qubits to the parent since you compute it if its None based on M. Then M is made single value in a list passed as parameters to the parent - this is kinda the equivalent of what params is to the StatePreparation. You can see how its handled there.

As to ddt you are not using it here. You can parameterize the test and even create the data so you do not need a for loop rather ddt will create a single test for each data entry there. That is better as all the tests will always run - in the for loop, as you have it, it will stop at the first failure. Usually we like to run all tests and if a test had independent parts like this, but is in one test, it would be done with the self.subTest. This is something that can be sorted once the code is running properly - I think what I am suggesting above will fix things.

[Hirmay]

I have added almost every suggestion (handled even @1ucian0 's last suggestion), if you get time, please take a look at this. Thanks in advance XD!

[Cryoris]

Thanks for taking care of the comments! There's some more steps we can take to clean up the code, then it looks good to go from my side 🙂

[Hirmay]

Hi @Cryoris, I have incorporated all the changes you suggested, and it will pass all the tests now. The inverse function in the latest release of Qiskit, caused one test to fail, but have fixed it now. Can you please look at it and see if there's anything else required?

[Cryoris]

Thanks for all the work and incorporating all the changes! This looks great 🙂

[Hirmay]

Thank you so much to you, Jake, Luciano, and especially Steve, for going through my amateurishly written code and helping bring it to this stage, despite your incredibly busy schedules.

Steve, I'm incredibly grateful for the support and assistance you've provided throughout this journey. Without you, it wouldn't have been possible.

I'll probably bother you guys again with another pull request 😅, hopefully next time it'll be easier to deal with.

[woodsp-ibm]

Hirmay, you are very welcome. Congratulations on your successful contribution to Qiskit.

[Hirmay]

Thanks, again, Steve! XD