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

The synthesis function synth_cnot_count_full_pmh returns invalid circuits for certain section_size values #12106

Closed
alexanderivrii opened this issue Mar 31, 2024 · 12 comments · Fixed by #12588
Assignees
Labels
bug Something isn't working good first issue Good for newcomers synthesis
Milestone

Comments

@alexanderivrii
Copy link
Contributor

Environment

  • Qiskit version: 1.0
  • Python version: 3.10
  • Operating system: Windows

What is happening?

The following code

from qiskit.synthesis import synth_cnot_count_full_pmh
from qiskit.synthesis.linear import random_invertible_binary_matrix

mat = random_invertible_binary_matrix(5, seed=1234)
qc = synth_cnot_count_full_pmh(mat, section_size=6)

produces an empty circuit.

How can we reproduce the issue?

Run the code above.

What should happen?

Setting section_size to a large number would probably produce suboptimal results, however it should still produce a valid (non-empty) circuit.

Any suggestions?

I believe that something wrong is going on when section size is larger than the size of the matrix, for instance in the above example setting section_size to a number 5 or lower does work correctly.

@alexanderivrii alexanderivrii added the bug Something isn't working label Mar 31, 2024
@alexanderivrii alexanderivrii added this to the 1.1.0 milestone Mar 31, 2024
@alexanderivrii alexanderivrii added the good first issue Good for newcomers label Mar 31, 2024
@github-project-automation github-project-automation bot moved this to Tagged but unassigned in Contributor Monitoring Mar 31, 2024
@Tarun-Kumar07
Copy link
Contributor

Hey @ShellyGarion and @alexanderivrii, I'm new to the Qiskit code base and currently exploring issues to contribute to. I noticed this one and I'm interested in working on it. Would that be possible?

@ShellyGarion
Copy link
Member

thanks @Tarun-Kumar07 - I've assigned you on this issue. Please let us know when you open a PR.

@Tarun-Kumar07
Copy link
Contributor

Hey @ShellyGarion, I'm uncertain about the expected behavior here.
The synth_cnot_count_full_pmh implements the algorithm mentioned in Efficient Synthesis of Linear Reversible Circuits. According to the paper, we use section_size to partition the provided matrix.
Therefore, I believe section_size > num_qubits is not correct. Should I throw an exception in this case?

@ShellyGarion
Copy link
Member

@Tarun-Kumar07 - Indeed, using section_size > num_qubits is not correct.
There are two options:

  1. Raise a QiskitError and add a test that checks it.
  2. When section_size > num_qubits set section_size = num_qubits and return the QuantumCircuit obtained in this case.

@Tarun-Kumar07
Copy link
Contributor

I believe option 1 is preferable, as it provides clarity to the user by immediately alerting them to any violation of the condition

@ShellyGarion
Copy link
Member

I believe option 1 is preferable, as it provides clarity to the user by immediately alerting them to any violation of the condition

OK, so please go ahead with the fix, and let me know when you open a PR

@Tarun-Kumar07
Copy link
Contributor

Hey @ShellyGarion , I have opened a PR #12166.
Can you please have a look at it.
Thanks

@Tarun-Kumar07
Copy link
Contributor

Hey @ShellyGarion,

Is there another task or issue I can contribute to? I've been searching through the good first issues but haven't found something suitable to work on.

@ShellyGarion
Copy link
Member

Hey @ShellyGarion,

Is there another task or issue I can contribute to? I've been searching through the good first issues but haven't found something suitable to work on.

Indeed, the best way is to ask in Qiskit slack channels (as you did). Good luck!

@ShellyGarion ShellyGarion modified the milestones: 1.1.0, 1.2.0 Apr 18, 2024
@ShellyGarion ShellyGarion changed the title The synthesis function synth_cnot_count_full_pmh returns empty circuit when section_size exceeds the number of qubits The synthesis function synth_cnot_count_full_pmh returns invalid circuits for certain section_size values May 2, 2024
@ShellyGarion
Copy link
Member

Following the discussion in #12166 we observed that for some values of section_size we get wrong circuits.
In addition, we would like to choose the optimal section_size as a parameter of the number of qubits.

@Abdalla01001
Copy link
Contributor

Abdalla01001 commented Jun 28, 2024

@ShellyGarion @alexanderivrii pls, can you assign me? I want to give it a look.

@ShellyGarion
Copy link
Member

@ShellyGarion @alexanderivrii pls, can you assign me? I want to give it a look

Thanks @Abdalla01001, but please note that there is already an open PR #12166 by @Tarun-Kumar07.
Could you please take a look at the discussion there and see if you can help?

@github-project-automation github-project-automation bot moved this from Assigned to Done in Contributor Monitoring Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment