Skip to content
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

Add Heavy square code to generators #313

Merged
merged 23 commits into from
Aug 23, 2021
Merged

Conversation

nahumsa
Copy link
Contributor

@nahumsa nahumsa commented Apr 29, 2021

Related to #150.

Added implementation of heavy hex, the input is the code distance, d. In the implementation I chose to add nodes referring to each type of qubit used in the Heavy Hex Code (data, syndrome, and flag) which may cause the node order to be confusing, but that was the best way that I figure out how to implement.

nahumsa and others added 4 commits March 31, 2021 22:56
Added implementation of [heavy hex](https://journals.aps.org/prx/abstract/10.1103/PhysRevX.10.011022), the input is the code distance, d. In the implementation I chose to add nodes referring to each type of qubit used in the Heavy Hex Code (data, syndrome, and flag) which _may cause the node order to be confusing_, but that was the best way that I figure out how to implement.
@nahumsa nahumsa changed the title Heavy square code Add Heavy square code to generators Apr 29, 2021
@coveralls
Copy link

coveralls commented Apr 29, 2021

Pull Request Test Coverage Report for Build 1160023027

  • 215 of 220 (97.73%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 97.518%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/generators.rs 215 220 97.73%
Totals Coverage Status
Change from base Build 1159928934: 0.07%
Covered Lines: 9706
Relevant Lines: 9953

💛 - Coveralls

@mtreinish mtreinish added this to the 0.10.0 milestone Jun 4, 2021
Copy link
Contributor

@ecpeterson ecpeterson left a comment

Choose a reason for hiding this comment

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

Rust is not my native language, so I have no comments about style, but the QEC-related stuff is all correct. I did leave some comments about legibility of intent.

src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
@@ -983,6 +983,139 @@ pub fn directed_grid_graph(
})
}

/// Generate an undirected heavy square graph.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a variant of the heavy square lattice (see Fig 10.b, right-hand side) whose edge case handling (lines 1054-1075 in the present PR) is a bit different. No comment on whether it's worth including as an option, but it is definitely worth mentioning in the docstring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestion how this can be mentioned in the docstring?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only important thing is to bring to the reader's attention that there is another variant and to disambiguate which you mean. So, something like:

NOTE: This function generates the four-frequency variant of the heavy square code. See Fig 10.b left of /1907.09528 , and compare Fig 10.b right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The note was added on 60a150b.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this. Same as on heavy hex the only question I have before merging is do you think it makes sense to add a directed version too? If so I think that would be useful to have.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Looking at the figure 10 in the paper I think this has the same direction issues as #293

Screenshot_2021-08-17_12-58-13
Screenshot_2021-08-17_12-58-35

Other than that I think this is good to go.

src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
src/generators.rs Outdated Show resolved Hide resolved
@mtreinish mtreinish mentioned this pull request Aug 17, 2021
4 tasks
@lcapelluto lcapelluto self-assigned this Aug 20, 2021
Copy link
Contributor

@ecpeterson ecpeterson left a comment

Choose a reason for hiding this comment

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

With @mtreinish's amendments, this also LGTM.

@mtreinish
Copy link
Member

LGTM, just as on #293 I took care of applying the last suggestions and fixing the tests to include this in the release. Thanks for adding these generators @nahumsa both this and heavy hex will be really nice to integrate with qiiskit-terra's CouplingMap after 0.10.0 goes out so we can have an a qiskit api to generate heavy hex and heavy square coupling maps.

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.

None yet

5 participants