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

Stop using faer-rs for uc_gate.rs #13956

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mtreinish
Copy link
Member

@mtreinish mtreinish commented Mar 4, 2025

Summary

This commit stops using faer and ndarray for the uc_gate.rs module and switches to use nalgebra instead. The uc_gate.rs module works all in terms of 2x2 matrices and moving to nalgebra brings the advantage of having everything be fixed size stack allocated for the arrays used. While this code path isn't performance critical this should improve performance, despite the goal here being to remove a dependency on faer.

The tradeoff here is that nalgebra doesn't seem to have a general eigensolver natively. There is functionality for this in the nalgebra-lapack crate, but as the name implies this depends on linking against lapack to work. The reason we originally started using faer is because it's pure rust and doesn't complicate our build system. In this specific case this is not a big deal because the matrices we need to compute the eigenvectors and eigenvalues for is a 2x2 so this commit includes a function to compute that directly. This lack of functionality is something to keep in mind for the future if we need this functionality more generally we will have to make a choice between faer and linking against lapack.

Details and comments

Part of #13665

Co-authored-by: Gadi Aleksandrowicz [email protected]
Co-authored-by: Alexander Ivrii [email protected]

This commit stops using faer and ndarray for the uc_gate.rs module and
switches to use nalgebra instead. The uc_gate.rs module works all in
terms of 2x2 matrices and moving to nalgebra brings the advantage of
having everything be fixed size stack allocated for the arrays used.
While this code path isn't performance critical this should improve
performance, despite the goal here being to remove a dependency on faer.

The tradeoff here is that nalgebra doesn't seem to have a general
eigensolver natively. There is functionality for this in the
nalgebra-lapack crate, but as the name implies this depends on linking
against lapack to work. The reason we originally started using faer is
because it's pure rust and doesn't complicate our build system. In this
specific case this is not a big deal because the matrices we need to
compute the eigenvectors and eigenvalues for is a 2x2 so this commit
includes a function to compute that directly. This lack of functionality
is something to keep in mind for the future if we need this
functionality more generally we will have to make a choice between faer
and linking against lapack.

Part of Qiskit#13665

Co-authored-by: Gadi Aleksandrowicz <[email protected]>
Co-authored-by: Alexander Ivrii <[email protected]>
@mtreinish mtreinish added Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Mar 4, 2025
@mtreinish mtreinish added this to the 2.1.0 milestone Mar 4, 2025
@mtreinish mtreinish requested a review from a team as a code owner March 4, 2025 20:53
@qiskit-bot
Copy link
Collaborator

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

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13662732606

Details

  • 70 of 77 (90.91%) changed or added relevant lines in 2 files are covered.
  • 10 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.007%) to 87.11%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/uc_gate.rs 69 76 90.79%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 2 94.39%
crates/qasm2/src/lex.rs 2 92.48%
crates/qasm2/src/parse.rs 6 96.68%
Totals Coverage Status
Change from base Build 13661892985: 0.007%
Covered Lines: 76468
Relevant Lines: 87783

💛 - Coveralls

.into_iter()
.map(|x| x.as_array().to_owned())
.map(|x| {
let res: MatrixView2<Complex64> = x.try_as_matrix().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, we should change this to let res: MatrixView2<Complex64, Dyn, Dyn> = x.try_as_matrix().unwrap();, otherwise the conversion fails.

@@ -286,7 +286,7 @@ def _dec_ucg_help(self):
This method finds the single qubit gate arising in the decomposition of UCGates given in
https://arxiv.org/pdf/quant-ph/0410066.pdf.
"""
single_qubit_gates = [gate.astype(complex) for gate in self.params]
single_qubit_gates = [np.asarray(gate, dtype=complex, order="f") for gate in self.params]
Copy link
Member

Choose a reason for hiding this comment

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

Or does this line make the rust code work without needing the Dyn trait?

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 Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants