-
Notifications
You must be signed in to change notification settings - Fork 0
Integrate PermRowCol #43
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
Conversation
783ed86 to
2b7fc70
Compare
synir/tests/clifford_tableau.rs
Outdated
| @@ -239,3 +242,55 @@ fn test_custom_clifford_synthesis_simple() { | |||
| let ref_ct = parse_clifford_commands(3, mock.commands()); | |||
| assert_eq!(clifford_tableau, ref_ct); | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by different file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are specific to perm row col, not so much clifford_tableaus themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understandable, all synthesis tests for the specific IR are stored inside the IR type itself. i.e. The naive, permrowcol etc. tests are stored in clifford_tableau.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that's probably not very suitable in the long run when we might have many different synthesizer algorithms for the same data structure. But I guess for now, we can keep it and put it as part of the larger restructuring/renaming that needs to be done because its unclear where what is defined from filestructure alone anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, adjusted file structure.
bedc693 to
9dbcd9c
Compare
458b16c to
2269db0
Compare
Moved to StableGraph, remove iterations from 0..graph_size as graphs no longer compact. Ensure unweighted edges have a defaulta weight of 1. Add helper function to find edge and node count of Connectivity. Remove asserts to check if node index is less than node count as graphs are no longer compact.
Mixup between rows and columns fixed. Added functionality to perform x and z cleanup interchangeably.
Add destabilizer and stabilizer to extract PauliLetter at certain position along particular qubits stabilizer/destabilizer. Add columns to get iterator over PauliColumn.
Add clean_prc for X and Z. Add pick_column and pick_row.
Remove unused imports. Switch deprecated functions to new names. Remove uneeded references.
…lCliffordSynthesizer
|
I wrote this basic SWAP -> identity + permutation test, but it fails: |
|
For some reason I cannot reply to my own comment. But a similar thing happens with the 2CNOT -> 1 CNOT + permutation case. In both directions: |
|
The CT synthesis tests should be refactored that they check some basic synthesis properties for ALL synthesizers - not just NaiveSynthesis - in python you can do this with parametrized tests and give it different options. Not sure if rust can do that too. I'm mainly talking about |
|
Fixed the incorrect column weight calculations, which should ignore the effect of terms along the chosen pivot row. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this file here? The crate has been using self named module files so far, not mod.rs files. Not saying one is better than the other, just striving for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unable to get this to work in the tests folder. I don't know why, but maybe the new file structure does not yet work in the integration tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it doesn't work because each file in the tests folder (but not subfolders) is compiled as a separate crate. So with self named module files, we need a common.rs at the top level next to the common folder, but then Rust tries to compile this file as a crate which doesn't work (for this, the referenced mock_circuit and sample_clifford_tableaus would need to be on the same level). With the old naming, all module files are within the common subfolder and hence ignored, so it works.
Source: https://doc.rust-lang.org/book/ch11-03-test-organization.html#submodules-in-integration-tests
Also, sharing functionality like this is generally discouraged and shared test functionality should be extracted to a crate that is then added as dev-dependency to the original crate. But I'd not do this for now.
| // #[derive(Default)] | ||
| pub struct PermRowColCliffordSynthesizer { | ||
| connectivity: Connectivity, | ||
| permutation: Vec<usize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be documented how the permutation is stored:
out[i] = in[perm[i]], i.e., specifies where in the output an axis comes fromout[perm[i]] = in[i], i.e., specifies where an input axis goes to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This needs to be fixed, double checked, and documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, this documentation should now go to CliffordTableau.get_permutation()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation added.
|
For the integration tests, I would use parameterized tests, which are done using macros. A simple declarative macro would be macro_rules! test_clifford {
($fun:ident, $expected:expr) => {
paste::item! {
#[test]
fn [< synthesize_ $fun >]() {
let clifford_tableau = $fun();
let (mock, new_ct) = run_synthesizer(&clifford_tableau);
assert_eq!(mock.commands(), $expected);
check_mock_equals_clifford_tableau(&clifford_tableau, &mock, new_ct.get_permutation());
}
}
};
}
test_clifford!(sample_s_gate, &[MockCommand::S(0)]);
test_clifford!(sample_s_dgr_gate, &[MockCommand::S(0), MockCommand::Z(0)]);using Alternatively, we can use a procedural macro from a crate. Could look like use parameterized::parameterized;
#[parameterized(fun = {
sample_s_gate,
sample_s_dgr_gate
}, expected = {
&[MockCommand::S(0)],
&[MockCommand::S(0), MockCommand::Z(0)]
})]
fn synthesis(fun: fn() -> CliffordTableau, expected: &[MockCommand]) {
let clifford_tableau = fun();
let (mock, new_ct) = run_synthesizer(&clifford_tableau);
assert_eq!(mock.commands(), expected);
check_mock_equals_clifford_tableau(&clifford_tableau, &mock, new_ct.get_permutation());
}though I'm not super satisfied with |
|
Yeah, I don't like the naming of |
Update clean_observable functions to apply an additional CNOT to set identity pivots to Z or X respectively. This removes onus of custom callback to always provide valid ordering. Cleaned up code slightly to remove unused variables and imports. Removed unneeded return functions, into_iter calls. Added documentation on updated functions.
The additional CNOTs seen are included because the chosen pivot is the identity. To avoid this, we add the cost of the additional CNOT to the pick_column heuristic. This breaks ties in our favor for symmetric setups.
The custom callback sets the chosen pivot_row and pivot_column to be the pivot qubit and thus determines the non-identity term in the horizontal and vertical identity rows. Thus, we should allow synthesis up to permutation if we have off-diagonal pivot_row and pivot_column possibilities.
Remove unneeded return
TODO:
CliffordTableua.get_permutation() -> Option<Vec<usize>>that calculates the permutation if the tableau is a permutation matrix, otherwise returnNone.mod.rsin the folders. @Aerylia Could not get this to work earlier.