Add Rust representation for most controlled gates #12659
Conversation
ElePT
added
Rust
Pull Request Test Coverage Report for Build 9676345425Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
ElePT
added
the
mod: circuit
…ircuit_instruction.rs. Add unit test.
Pull Request Test Coverage Report for Build 9745926823Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 9762974381Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
crates/circuit/src/operations.rs
Outdated
| @@ -305,13 +303,13 @@ static STANDARD_GATE_NAME: [&str; STANDARD_GATE_SIZE] = [ | |||
| "cu", // 39 | |||
| "cu1", // 40 | |||
| "cu3", // 41 | |||
| "c3x", // 42 | |||
| "c3x", // 42 ("mcx") | |||
There was a problem hiding this comment.
I feel like this mismatch will cause a problem eventually. Like it's not an issue right now because we never use these gates out of the box anywhere. But we use these names as the canonical identifier in a lot of places (which is why the overloaded mcx name is a real problem). For example, when we port the qasm exporters to rust they'll just call op.name() internally and then this will return c3x instead of mcx which will cause issues.
There was a problem hiding this comment.
I agree. But it seems like the solution would be not using the overloaded mcx name in Python, wouldn't it?
There was a problem hiding this comment.
It would, and ideally that's how I would expect us to fix this. But when we discussed potentially doing that as part of #12628 I thought the concern was around backwards compatibility and the consensus was we shouldn't change the name until 2.0?
There was a problem hiding this comment.
That's what I vaguely remembered. If we want to rename the gates for 2.0, should we then introduce deprecation warnings in 1.2? I am not sure what the protocol is with name changes, we can't add the new path in advance, right?
There was a problem hiding this comment.
hah, this just became a problem after the recent commits: https://github.com/Qiskit/qiskit/actions/runs/9764823435/job/26954059268?pr=12659#step:8:16894
There was a problem hiding this comment.
The deprecation path for something like this isn't clear as it's a straight change in behavior. There isn't a good way to signal this to users unless we emit a FutureWarning every time a C3XGate is created. This might be the case we just document this in the 2.0 release notes and say it wasn't easy to raise a deprecation warning for.
There was a problem hiding this comment.
Ok, I didn't see a way around the recent failures other than renaming "c3x" to "mcx" in rust space (f05c9a1). I don't love it, but I guess we can then change both names at the same time?
Pull Request Test Coverage Report for Build 9765116170Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9773297349Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9780967332Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9783170656Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
|
||
| SKIP_LIST = {"rx", "ry", "ecr"} | ||
| CUSTOM_MAPPING = {"x", "rz"} | ||
| CUSTOM_NAME_MAPPING = {"mcx": C3XGate()} | ||
| MATRIX_SKIP_LIST = {"c3sx"} |
There was a problem hiding this comment.
I thought I saw the rust definition of this matrix using the macro in the code?
There was a problem hiding this comment.
Yes, the test is being skipped because of the python definition, not the rust one.
qiskit.circuit.exceptions.CircuitError: "to_matrix not defined for this <class '_SingletonC3SXGate'>"
There was a problem hiding this comment.
Hmm, that seems like a bug in the python side then. But lets fix that in a separate PR
There was a problem hiding this comment.
I will try to find a fix tomorrow in a separate PR.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Pull Request Test Coverage Report for Build 9842649589Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
* Add C3X (MCX), extend rust tests to multi-controlled gates. * Add macro to generate multi-controlled gates. Add CU, CU1, CU3, C3SX, C4X, CCZ. * Kill C4XGate * Finish adding gates, add circuit construction methods when possible. * Add import paths, fix drawer test. * Establish CGates with non-default control states as non-standard in circuit_instruction.rs. Add unit test. * Fix merge conflicts * Apply macro on missing gates * Add RCCX gate and RC3X (RCCCX) gate. * Make equivalence tests more explicit * Fix lint * Modify circuit methods for consistency * Fix default ctrl state for 3q+ gates, add test for CCZ * Apply comments from Matt's code review * Fix ctrl_state logic * Rename c3x to mcx? * Brackets didn't match explanation * Make sure controlled test doesn't use custom ControlledGate instances. * Rename c4x to mcx in Rust space. * Return PyResult rather than panic on error * Add suggestion from Matt's code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> --------- Co-authored-by: John Lapeyre <jlapeyre@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
This PR adds Rust implementations for most controlled gates left in #12566.
CUGateCU1GateCU3GateC3XGateC3SXGateCCZGateRCCXGateRC3XGateThis is done through a newly implemented
make_n_controlled_gatemacro (which is currently limited to adding controls to a single-qubit gate).Details and comments
I attempted to implement C4XGate but got some issues with the implementation of the FixedIntializer traits for arrays of more than 16 elements. There are workarounds, but given its low priority the implementation has been left for a follow-up.