Skip to content

feat: pg-optimise - #1943

Open
yao-cqc wants to merge 2 commits into
mainfrom
yt/pg-optimise
Open

feat: pg-optimise#1943
yao-cqc wants to merge 2 commits into
mainfrom
yt/pg-optimise

Conversation

@yao-cqc

@yao-cqc yao-cqc commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

This PR introduces two passes: GroupCommutingOpsPass and RotationMergingPass. See the README for details.

@yao-cqc
yao-cqc marked this pull request as ready for review August 20, 2026 12:15
@yao-cqc
yao-cqc requested a review from a team as a code owner August 20, 2026 12:15
@yao-cqc
yao-cqc requested review from cqc-alec and a lite review from Copilot and removed request for Copilot August 20, 2026 12:15

@cqc-alec cqc-alec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Just a few comments and queries.

Comment on lines +15 to +16
tableaux.
Consider running `CanonicalFormPass` before either optimisation pass so that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
tableaux.
Consider running `CanonicalFormPass` before either optimisation pass so that
tableaux.
Consider running `CanonicalFormPass` before either optimisation pass so that


## Passes

- `GroupCommutingOpsPass`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we should try to be consistent about using noun phrases or verb phrases in pass names. Unfortunately tket2 is already inconsistent, so not sure what to suggest!

Comment on lines +40 to +41
/// The strings must use opposite encodings: one must have been packed with
/// `z_first` set to `true`, and the other with it set to `false`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems like a weird condition. Is there a good reason for it? Can we not just settle on one convention (either X or Z first), and have this function handle it?

z_first,
}
}
fn flip_to_zx(&mut self) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would coerce_to_zx be a better name? Since it might already be Z-first? (And in that case, should we short-circuit the manipulation?)

Comment on lines +204 to +205
let p0 = vec![Pauli::X, Pauli::I, Pauli::Z];
let p1 = vec![Pauli::X, Pauli::I, Pauli::Z];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These are the same, is that intentional?

data: RotationData::new(vec![Pauli::X, Pauli::I], 0.4),
});
let merged_pg = rotation_merging(&pg);
assert_eq!(merged_pg.get_ops().len(), 2);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this not 1?

let merged_pg = rotation_merging(&pg);

assert_eq!(merged_pg, pg);
assert!(compare_unitaries_via_tk(&pg, &merged_pg));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is redundant given the assertion above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants