Skip to content

Fix UnitarySynthesis to respect synth_gates (backport #14345) #14351

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
Merged
merged 2 commits into from
May 13, 2025
Merged

Fix UnitarySynthesis to respect synth_gates (backport #14345) #14351

merged 2 commits into from
May 13, 2025

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented May 12, 2025

Fixes #14343

Summary

This commit fixes UnitarySynthesis. It has a parameter synth_gates, which is meant to be a list of gates that are synthesized. However, the previous implementation did not respect this parameter, and only synthesized the gates named unitary. This commit fixes this issue by checking if gate's name is in synth_gates.

Details and comments

When a non-default method is specified, the DAG nodes are traversed in UnitarySynthesis._run_main_loop. There, the name of the node is checked, and in the original implementation, the operation is synthesized if the name is unitary. This PR fixes this condition so that the synthesis takes place if the name is in synth_gates.

When the method argument is not given, UnitarySynthesis invokes the rust version of run_main_loop, which basically has the same structure. The PR also fixes it.

This is an automatic backport of pull request #14345 done by Mergify.

@yoshum @mergify
Fix UnitarySynthesis to respect synth_gates (#14345)
6e8af42 
* Fix UnitarySynthesis to respect synth_gates

This commit fixes UnitarySynthesis. It has a parameter `synth_gates`,
which is meant to be a list of gates that are synthesized. However,
the previous implementation did not respect this parameter, and only
synthesized the gates named `unitary`. This commit fixes this issue by
checking if gate's name is in `synth_gates`.

* Add tests for UnitarySynthesis' synth_gates

* Fix the default Rust impl of UnitarySynthesis

* Add the release note for #14345

(cherry picked from commit 0c2ce64)

# Conflicts:
#	crates/accelerate/src/unitary_synthesis.rs
@mergify mergify bot requested a review from a team as a code owner May 12, 2025 19:20
@mergify mergify bot added the conflicts used by mergify when there are conflicts in a port label May 12, 2025
@mergify Mergify
Copy link
Contributor Author

mergify bot commented May 12, 2025

Cherry-pick of 0c2ce64 has failed:

On branch mergify/bp/stable/2.0/pr-14345
Your branch is up to date with 'origin/stable/2.0'.

You are currently cherry-picking commit 0c2ce6488.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   qiskit/transpiler/passes/synthesis/unitary_synthesis.py
	new file:   releasenotes/notes/fix-UnitarySynthesis-synth_gates-4e084db18ece8bfe.yaml
	modified:   test/python/transpiler/test_solovay_kitaev.py
	modified:   test/python/transpiler/test_unitary_synthesis.py

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   crates/accelerate/src/unitary_synthesis.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@github-actions github-actions bot added Changelog: Bugfix Include in the "Fixed" section of the changelog synthesis Community PR PRs from contributors that are not 'members' of the Qiskit repo labels May 12, 2025
@github-actions github-actions bot added this to the 2.0.1 milestone May 12, 2025
@yoshum
Copy link
Contributor

yoshum commented May 13, 2025

I'll take care of the conflict. In the main branch from which the commit in this PR is cherry-picked, py_run_main_loop has been renamed to run_unitary_synthesis and made public during the process of reorganizing the crates. This change seems irrelevant to stable/2.0, so I'll undo the name change here.

@yoshum
Copy link
Contributor

yoshum commented May 13, 2025

How can I contribute to it? Can anyone give me the permission to update the PR, or should I create a new PR to stable/2.0?

@alexanderivrii
Copy link
Member

alexanderivrii commented May 13, 2025 •
edited
Loading

Note that the PR's name is mergify/bp/stable/2.0/pr-14345. I am not sure if this is fully correct and complete, but I would do something as follows:

git fetch upstream
git checkout mergify/bp/stable/2.0/pr-14345

<change code>

git push

Can you please check if this works for you?

@yoshum
Copy link
Contributor

yoshum commented May 13, 2025

No, I tried it but got the following error:

remote: Permission to Qiskit/qiskit.git denied to yoshum.
fatal: unable to access 'https://github.com/Qiskit/qiskit.git/': The requested URL returned error: 403

From this and this document, I suspect that explicit permission from Qiskit's maintainers is necessary for me to directly push to the PR branch.

@alexanderivrii
Copy link
Member

Thanks @yoshum. I was not sure about the process myself, but apparently the correct way is for you to open a PR targeting the mergify/bp/stable/2.0/pr-14345 branch.

@yoshum
Copy link
Contributor

yoshum commented May 13, 2025

Thank you! I'll try it.

@yoshum yoshum mentioned this pull request May 13, 2025
Merged
@coveralls
Copy link

Pull Request Test Coverage Report for Build 15002139511

Details

  • 7 of 8 (87.5%) changed or added relevant lines in 2 files are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.002%) to 88.125%
Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/unitary_synthesis.rs 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.81%
crates/qasm2/src/lex.rs 3 92.48%
crates/qasm2/src/parse.rs 6 97.61%
Totals Coverage Status
Change from base Build 14931000174: -0.002%
Covered Lines: 72928
Relevant Lines: 82755
💛 - Coveralls

@alexanderivrii alexanderivrii added this pull request to the merge queue May 13, 2025
Merged via the queue into stable/2.0 with commit c895e03 May 13, 2025
26 checks passed
@jakelishman jakelishman deleted the mergify/bp/stable/2.0/pr-14345 branch June 9, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderivrii alexanderivrii alexanderivrii approved these changes

Assignees
No one assigned
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo conflicts used by mergify when there are conflicts in a port synthesis
Projects
Contributor Monitoring
Status: Done
Milestone
2.0.1
Development

Successfully merging this pull request may close these issues.
4 participants
@qiskit-bot @yoshum @alexanderivrii @coveralls