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

Fixes issue #12977 #13679

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Fixes issue #12977 #13679

wants to merge 10 commits into from

Conversation

Ak-ash22
Copy link
Contributor

Summary

Updates the synthesis methods to use the PhaseGate instead of the IBM-legacy U1Gate.

Details and comments

Substituted the CU1/MCU1Gate for the CPhase/MCPhaseGate in the qiskit/synthesis/multi_controlled/mcx_synthesis.py file.

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 17, 2025
@Cryoris
Copy link
Contributor

Cryoris commented Jan 17, 2025

Thanks for opening the draft 🙂 The tests are failing because they were expecting the u1 gates (or cu1/mcu1) and now the phase gates are used. You can just update those tests to now expect the phase gates.

Could you include a releasenote which mentions this gate change for certain MCX gates under the upgrades section?

@Cryoris Cryoris added the Changelog: API Change Include in the "Changed" section of the changelog label Jan 17, 2025
@Ak-ash22
Copy link
Contributor Author

Hey @Cryoris ... can you help me navigate through these tests that are breaking? For ex:

  1. One of the test that breaks is test_decompose_mixture_of_names_and_labels() , and the only possible line of code I could track here is this self.assertEqual(dag.op_nodes()[5].name, "cu1"), am I just supposed to change "cu1" to "cphase"?
  2. Also in test_mcx_gate_variants(), you would expect me to change the expected_qasm() line, to expect cphase instead, right?

I just wanted to confirm before I went ahead with these, as this is my second time contributing.

@alexanderivrii
Copy link
Contributor

Hi @Ak-ash22, you are correct: you need to modify the failing tests to refer to CPhase instead of CU1.

For test_decompose_mixture_of_names_and_labels(): the name of the CPhase gate is "cp", so you should change the test to self.assertEqual(dag.op_nodes()[5].name, "cp").

For test_mcx_gate_variants(): you should indeed alter the expected_qasm string.

In case you do not know this already, you can click on "Details" right next to the "Tests/macOS-..." line above, taking you to the "Run tests" log, which shows both how many and which tests have failed, and also the specific error messages (right at the bottom of the log). For test_decompose_mixture_of_names_and_labels() you see

    AssertionError: 'cp' != 'cu1'
- cp
+ cu1

which indeed indicates that the gate name's is "cp".

@Ak-ash22
Copy link
Contributor Author

Hello @alexanderivrii @Cryoris .. Can you help me solve this particular issue?... I have modified the failed tests to use cp instead of cu1 which was successful for 2 of the tests.
The other 2 tests, where I have to alter the expected_qasm string is not being solved.

The expected_qasm string has cu1 and also mcu1. I tried changing both to cp and mcp which didnt work. I also tried changing only cp to cu1, which is not working as well.

@Cryoris
Copy link
Contributor

Cryoris commented Jan 24, 2025

These QASM output files can change substantially because it now dumps the decomposition via the MC-Phase path. So we don't only have to change the definition of mcu1 to mcp but also the entire decomposition of the mcp, which is not a native gate in QASM. Since all other tests pass, you can just print the QASM output of the current circuit and update the expected QASM accordingly.

@coveralls
Copy link

coveralls commented Feb 2, 2025

Pull Request Test Coverage Report for Build 13102387676

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.01%) to 88.816%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 92.97%
crates/qasm2/src/lex.rs 6 91.73%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 13072040346: -0.01%
Covered Lines: 79762
Relevant Lines: 89806

💛 - Coveralls

@Ak-ash22
Copy link
Contributor Author

Ak-ash22 commented Feb 2, 2025

Hello @alexanderivrii @Cryoris .. It took me a while but I managed to correct the expected_qasm string.
The main tests all seem to be running successfully, but now there is an issue with the lint test, although it rates 10/10 for the code.

The issue seems to be that some lines of expected_qasm strings are too long. Can you help me fix this one last issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants