Skip to content

Porting synth_qft_line to Rust #14161

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

Merged
merged 14 commits into from
Apr 29, 2025
Merged

Conversation

Barath-T
Copy link
Contributor

@Barath-T Barath-T commented Apr 2, 2025

Summary

This PR ports synth_qft_line in qiskit/synthesis/qft/qft_decompose_lnn.py to Rust (Closes #12245).

Details and comments

The functionality remains the same and no new optimisations or logic is introduced.

Comment needed: PR #13929 introduces SynthesisData, which would allow for a cleaner refactoring.

@Barath-T Barath-T requested a review from a team as a code owner April 2, 2025 17:53
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Apr 2, 2025
@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

@ShellyGarion ShellyGarion added synthesis Rust This PR or issue is related to Rust code in the repository labels Apr 3, 2025
@Barath-T Barath-T changed the title [WIP] Porting synth_qft_line to Rust Porting synth_qft_line to Rust Apr 3, 2025
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.

This looks very good! I only have two small comments below 🙂

@coveralls
Copy link

coveralls commented Apr 3, 2025

Pull Request Test Coverage Report for Build 14700264921

Details

  • 71 of 71 (100.0%) changed or added relevant lines in 6 files are covered.
  • 20 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 87.914%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 5 92.23%
qiskit/synthesis/permutation/permutation_reverse_lnn.py 15 31.82%
Totals Coverage Status
Change from base Build 14693758171: -0.01%
Covered Lines: 74393
Relevant Lines: 84620

💛 - Coveralls

Copy link
Member

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @Barath-T! I think this is in a good shape, modulo needing to adjust the Rust-level docstrings a bit.

Fyi, I have reworked #13929 to work with CircuitData directly (and in particular added convenience functions to append h, cp, cx gates), but for your PR keeping it in terms of LnnGatesVec is definitely better.

@Barath-T
Copy link
Contributor Author

Barath-T commented Apr 8, 2025

@alexanderivrii and @Cryoris Thank you for the review. I have updated the PR with the suggested changes. Please let me know if further refinements are needed. I'd also appreciate it if you can point me to other issues that i can contribute. Thank you in advance! :)

@alexanderivrii
Copy link
Member

alexanderivrii commented Apr 9, 2025

@Barath-T, sorry, my suggestion in the previous review was incorrect:
since

#[pyfunction]
#[pyo3(signature=(num_qubits, do_swaps=true, approximation_degree=0))]
pub fn synth_qft_line(

is treated as a Python function, it actually needs sphinx style documentation, exactly like you had previously (thanks to @Cryoris for explaining that to me). Could you please revert this change (but keep the one to append_h)? In some sense, this does not even matter, as the main documentation includes the one from synthesis/qft/qft_decompose_lnn.py and not the one from qft_decompose_lnn.rs, but it might be good to keep it correct just in case.

@Barath-T Barath-T force-pushed the port-synth_qft_line branch from c933dc4 to f4d0a59 Compare April 9, 2025 07:24
Copy link
Member

@alexanderivrii alexanderivrii left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! This looks good on my side, @Cryoris do you have any additional comments?

@Barath-T Barath-T requested a review from Cryoris April 14, 2025 06:50
@alexanderivrii alexanderivrii added this to the 2.1.0 milestone Apr 23, 2025
Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

LGTM. @Cryoris - do you have any further comments?

@ShellyGarion ShellyGarion added this pull request to the merge queue Apr 29, 2025
Merged via the queue into Qiskit:main with commit fc8d865 Apr 29, 2025
24 checks passed
@eliarbel eliarbel added the Changelog: None Do not include in changelog label Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo Rust This PR or issue is related to Rust code in the repository synthesis
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Port synth_qft_line to Rust
7 participants