-
Notifications
You must be signed in to change notification settings - Fork 82
[surface code] DataBlock refactor #1141
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
[surface code] DataBlock refactor #1141
Conversation
|
|
||
| class DataBlock(metaclass=abc.ABCMeta): | ||
| """A cost model for the data block of a surface code compilation. | ||
| """Methods for modeling the costs of the data block of a surface code compilation. |
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.
nice 👍
qualtran/surface_code/data_block.py
Outdated
| `self.n_cycles` to report the total number of cycles required. | ||
| """ | ||
|
|
||
| def n_cycles(self, n_logi_gates: 'MagicCount', logi_err_model: 'LogicalErrorModel') -> int: |
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.
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.
"logical" :D
naming things is hard, and -- in this module in particular -- there's a balance between ambiguity and readability. I'd like to use consistent, unambiguous names throughout. That means the variable names could get quite long if we don't abbreviate; so I'd like to abbreviate
num_physical_qubits -> n_phys_qubitsnum_logical_gates -> n_logi_gates
Also the MagicCount type annotation will change to GateCounts from qualtran.resource_counting which will make the variable name make more sense
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.
naming things is hard, and -- in this module in particular -- there's a balance between ambiguity and readability
you are the native english speaker in this conversation so I will let this to your judgement
Also the
MagicCounttype annotation will change toGateCounts
wouldn't that be confusing? since GateCounts could imply all gates including say cliffords?!
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.
this is an abstract interface, so yes -- it should generally be able to depend on the gate counts. If the implementation doesn't care about clifford counts it can ignore them. You can imagine a data block being able to do only a certain number of cliffords per cycle
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.
abbreviating logical -> logi seems very awkward to me as well. I'd have a preference towards using IDE to manage the longer names but avoid non-standard abbreviations, especially for logical -> logi.
Abbreviating by deleting letters of a word is also generally discouraged by the google style guide. Please consider using the full word for logical
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 mean "phys" and "prob" and "err" are already used everywhere
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've updated all the things within the scope of this PR to use full names
| """The number of surface code cycles to apply the number of gates to the data block. | ||
| Note that only the Litinski (2019) derived data blocks model a limit on the number of | ||
| magic states consumed per step. Other data blocks return "zero" for the number of cycles |
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.
"zero"
I don't think this is true ... the CCZ2T doesn't talk about this and focuses on routing and distillation as the dominant factors but it also prefly mentions the work needed to teleport the CCZ magic state without specifying how long it takes in page 11
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.
The distinction is if you have e.g. 50 factories in a litinski data block you're limited by the number of t states you can consume. If you're using a gidney model, you are not. They pay heed to the time it takes to transport the magic state, yes, but assume that that can be intelligently routed and parallelized so the magic state throughput (not time/latency) is unbounded.
With these models, there isn't "right" or "wrong" but rather fidelity to the original model, and I think in this case assuming unbounded t state consumption is closer to the original model than the restrictive 1-state-per-step. The gidney spreadsheet (in the supplementary materials) doesn't ever consider the data block being the time-limiting factor
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.
they way I have reconcilled these is by taking the max between generation and consumption cycles ... but I'm okay with saying it is zero for now
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.
the "identity value" for max is 0. The prior behavior, if you just want the generation cycle because you're doing gidney form the max might still choose the consumption as the limiting factor

This is the first part of polishing the API for surface code physical cost estimation. This PR deals with the data blocks. Changes:
DataBlockclass hierarchy. There is now one abstract base class and four implementations.Some shims are present to enable further refactoring:
QECSchemeandphys_errinto a little callableLogicalErrorModel.MagicCountin favor ofqualtran.resource_counting.GateCounts.