Skip to content

Implement Param add/mul as proper methods#15032

Open
Cryoris wants to merge 7 commits intoQiskit:mainfrom
Cryoris:param-add-mul
Open

Implement Param add/mul as proper methods#15032
Cryoris wants to merge 7 commits intoQiskit:mainfrom
Cryoris:param-add-mul

Conversation

@Cryoris
Copy link
Copy Markdown
Collaborator

@Cryoris Cryoris commented Sep 19, 2025

Summary

This commit implements the methods Param::add(_f64) and Param::mul(_f64), which previously existed as standalone functions in various places. This ensures we have a single place for these methods which is easily discoverable and cleans up the code.

Details and comments

This came up when implementing the C API for Param in #14837.

This also removes the single call to clone_param(&Param) -> Param which is already covered by Param::clone.

@Cryoris Cryoris requested a review from a team as a code owner September 19, 2025 09:32
@Cryoris Cryoris added Changelog: None Do not include in the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository labels Sep 19, 2025
@qiskit-bot
Copy link
Copy Markdown
Collaborator

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

  • @Qiskit/terra-core

Copy link
Copy Markdown
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

If we're going to raise these to Param, they need to be given names that clearly indicate at the call site that they do not actually support all Param instances, or we're just asking to make mistakes.

Comment thread crates/circuit/src/dag_circuit.rs
Comment thread crates/circuit/src/operations.rs Outdated
left.add(right).expect("Name conflict during add."),
))
}
_ => unreachable!("Unsupported addition."),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not unreachable. Unreachable isn't for marking "I don't want you to get here", it's for marking "this is literally impossible but the compiler can't reason about it".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yep, that's an oversight from copying it over 😅

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

A nicer way would be to have a proper error here -- we should also propagate the name-conflict error that we currently just .expect()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The panic is suitable if it could only have been caused by an internal logic error within the Rust code, but yeah, a non-aborting error for the name conflict would be nicer.

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 19, 2025

Pull Request Test Coverage Report for Build 17910214757

Details

  • 104 of 120 (86.67%) changed or added relevant lines in 9 files are covered.
  • 23 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+0.006%) to 88.283%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/operations.rs 85 101 84.16%
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/parameter/parameter_expression.rs 1 82.79%
crates/transpiler/src/passes/unitary_synthesis.rs 1 92.4%
crates/circuit/src/parameter/symbol_expr.rs 4 72.74%
crates/qasm2/src/lex.rs 5 91.75%
crates/qasm2/src/parse.rs 12 97.09%
Totals Coverage Status
Change from base Build 17859742834: 0.006%
Covered Lines: 92683
Relevant Lines: 104984

💛 - Coveralls

@jakelishman
Copy link
Copy Markdown
Member

The conflicts here are probably mostly related to the move to Rust 2024, especially the format update.

Do you want to include all the mathematical operations on numeric-like arguments that we identified in #14837, so that we can (in a follow up PR) then migrate the C API code of Param to use them?

@Cryoris
Copy link
Copy Markdown
Collaborator Author

Cryoris commented Oct 2, 2025

Yeah we can already include them in this PR 👍🏻 though if we move them here, then we should maybe directly update the QkParam class to use them, right?

@jakelishman
Copy link
Copy Markdown
Member

I don't mind if you want to do it in this PR, or move them over in a follow-up.

@jakelishman jakelishman added this to the 2.3.0 milestone Oct 8, 2025
@Cryoris Cryoris removed this from Qiskit 2.3 Dec 9, 2025
@Cryoris Cryoris modified the milestones: 2.3.0, 2.4.0 Dec 9, 2025
@github-project-automation github-project-automation Bot moved this to Ready in Qiskit 2.4 Dec 9, 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 the GitHub Release changelog. Rust This PR or issue is related to Rust code in the repository

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

5 participants