feat!: handling multiple QSystem gatesets#1370
Draft
jake-arkinstall wants to merge 3 commits intomainfrom
Draft
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1370 +/- ##
==========================================
- Coverage 83.84% 83.33% -0.52%
==========================================
Files 187 188 +1
Lines 28959 29347 +388
Branches 27880 28239 +359
==========================================
+ Hits 24282 24455 +173
- Misses 3517 3739 +222
+ Partials 1160 1153 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jake-arkinstall
force-pushed
the
feat/sol-gateset
branch
from
cfabaea to
4153afd
Compare
Collaborator
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
jake-arkinstall
changed the title
ss2165
reviewed
Jan 23, 2026
| /// Get the QSystemPlatform from the given string. Can be "Helios" or "Sol". | ||
| pub fn get_platform(platform: &str) -> Result<qsystem::QSystemPlatform> { | ||
| match platform { | ||
| "Helios" => Ok(qsystem::QSystemPlatform::Helios), |
Member
There was a problem hiding this comment.
nit: prefer lowercase strings, more natural in cli etc.
|
|
||
| @functools.cached_property | ||
| def phasedXX(self) -> ExtOp: | ||
| """Two-qubit XX gate (rpp).""" |
Member
There was a problem hiding this comment.
Suggested change
| """Two-qubit XX gate (rpp).""" | |
| """Two-qubit XX gate (alias 'rpp').""" |
etc., could be confusing for external viewers
| return self().get_op("PhasedXX").instantiate() | ||
|
|
||
| @functools.cached_property | ||
| def twinPhasedX(self) -> ExtOp: |
Member
There was a problem hiding this comment.
don't love the camel case for a property...all the old ones should be snake case too :/
| QSystemOp::PhasedXX => { | ||
| return Ok(EncodeStatus::Unsupported); | ||
| } | ||
| QSystemOp::TwinPhasedX => PytketOptype::NPhasedX, // TODO |
Contributor
Author
There was a problem hiding this comment.
This is for if OG Tket is to introduce a TwinPhasedX operation that is a special case of NPhasedX (and can't allow N!=2)
Member
There was a problem hiding this comment.
please add to the TODO comment 🙏
ss2165
reviewed
Jan 27, 2026
| /// PhasedX gate. | ||
| PhasedX, | ||
| /// ZZ gate with an angle. | ||
| ZZPhase, |
Member
There was a problem hiding this comment.
should this be marked helios-only?
ss2165
reviewed
Jan 27, 2026
Member
There was a problem hiding this comment.
needs a version bump, non-breaking I guess?
| /// Returns an error if the replacement fails. | ||
| pub fn lower_tk2_op(hugr: &mut impl HugrMut<Node = Node>) -> Result<Vec<Node>, LowerTk2Error> { | ||
| /// | ||
| /// TODO: consider a rename - tk2 can mean both 'tket2' and the TK2 gate. |
Member
There was a problem hiding this comment.
Suggested change
| /// TODO: consider a rename - tk2 can mean both 'tket2' and the TK2 gate. | |
| // TODO: consider a rename - tk2 can mean both 'tket2' and the TK2 gate. |
comment not docstring
| self.add_zz_phase(qb1, qb2, pi_2) | ||
| fn build_zz_max( | ||
| &mut self, | ||
| platform: QSystemPlatform, |
Member
There was a problem hiding this comment.
to me this suggests that the add methods belong to QSystemOp and the build ones should be implemented on some struct that contains both op and platform
the better encapsulation there might not be worth the refactor
jake-arkinstall
force-pushed
the
feat/sol-gateset
branch
from
957e987 to
873c817
Compare
Contributor
|
I noticed that it's emitting I can give you this specific example if you want it. |
Disambiguate helios and sol during rebasing, rename N2PhasedX -> TwinPhasedX Correct descriptions after the reintroduction of the rz gate Format + rust test fixups Rebase + document QSystemCodegenExtension::new Rename python testing of unimplemented sol features and change from pytest.raises to xfail Ruff format Remove redundant variable setting in xfail test Add unimplemented two-qubit operations for sol gateset (#1377) [decompositions.py](https://github.com/user-attachments/files/24907896/decompositions.py) Constructions should be as in attached python. --------- Co-authored-by: Jake Arkinstall <65358059+jake-arkinstall@users.noreply.github.com> Add missing maximally entangling XXPhase Update ZZPhase and ZZMax Correct output wire indexing for 2q sol gates, correct some typos in gate rebasing, add QFT test Correct add_phased_xx Apply requested changes, add addition test
jake-arkinstall
force-pushed
the
feat/sol-gateset
branch
from
873c817 to
b8fc25e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to explore the work required to add a second QSystem gateset within the framework of the existing gateset.
It is not complete, as rebasing work is required, but reviews will be extremely useful for knowing that this is going in the right direction.
The design choice is "least invasive". There is a good argument for a more invasive refactor to handle things naturally, but that is a longer term effort.
As a consequence of this least invasive approach, the
QSystemOpBuildertrait remains a trait, and thus now requires platform arguments for theaddandbuildfunctions (and for 1q operations this parameter is presently unused), which adds a lot of noise to the screen when reading and writing, and uses match blocks for when rebasing differs. I would like to explore handling this in a better manner.Another design choice is to bundle all gates into QSystem itself. The intent is to have user-facing guppy code fetch gateset-specific functions from a submodule of qsystem (perhaps re-exporting Helios gates from the parent module to retain backwards compatibility), but on the tket definition side they're still in one big "bag". This choice is made because of the overlap between the two gatesets' 1q operations (rxy, rz) and platform functions (qalloc etc), and the intent is that the more invasive refactor will take gateset overlaps into account while hopefully introducing more separation.
BREAKING CHANGE: QSystemOpBuilder trait's
build_*andadd_*functions take an additionalplatformargument. LowerTketToQSystemPass now has aplatformmember and no longer satisfies Default (a platform must be provided, and there is no good 'default' platform).