-
Notifications
You must be signed in to change notification settings - Fork 50
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
Use rotation eps
in GateCounts
#1255
base: main
Are you sure you want to change the base?
Use rotation eps
in GateCounts
#1255
Conversation
we had discussed binning by ceil(log(eps)) -- can you comment on that |
@tanujkhattar's comment:
Binning by the above integer approximation can be implemented. My bigger concern is the various epsilons floating around:
Should we remove this |
Certain physical cost functions make certain assumptions, and we've tried to implement them as described in the literature. The coupling between the logical costs and the physical cost models is done -- in this case -- via the methods on |
qualtran/surface_code/ui.py
Outdated
): | ||
"""Updates the visualization.""" | ||
if any(x is None for x in [physical_error_rate, error_budget, *algorithm_data, magic_count]): | ||
raise PreventUpdate | ||
|
||
# TODO: We implicitly rely on the order of the input components | ||
qubits, measurements, ts, toffolis, rotations, n_rotation_layers = algorithm_data | ||
|
||
rotation_eps = 1e-3 / rotations # TODO this should not be a magic number |
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 we support "unknown precision" rotations. I guess in the rotation bloqs we have a default eps; but it's not hard to imagine a situation where a researcher has a bunch of rotations and they haven't computed an epsilon for each of them
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 using error_budget / rotations
here
3e542af
to
0b7a9e3
Compare
@mpharrigan ptal! |
eps
in GateCounts
eps
in GateCounts
00a226b
to
9b45a0b
Compare
taking a look now. Can you update the original PR description with a summary of changes in this pr |
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.
some comments and questions, but I think this is generally the correct approach
binned_rotation_epsilons: Counter[int] = field( | ||
factory=Counter, converter=_mapping_to_counter, eq=lambda d: tuple(d.items()) | ||
) | ||
eps_bin_prec: int = 20 |
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.
please update the docstring with a thorough description of the representation of rotations.
Is it possible for someone to construct this class with inconsistent binned values vs the eps_bin_prec
? What if someone puts in the epsilons as floats instead of 2**20*floats
document that the eps_bin_prec
behavior during addition.
eps_bin_prec
-> epsilon_precision
? and then document that precision is measured by number of binary digits.
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.
Used np.format_scientific_float
to avoid such issues.
|
||
def __add__(self, other): | ||
if not isinstance(other, GateCounts): | ||
raise TypeError(f"Can only add other `GateCounts` objects, not {self}") | ||
|
||
eps_bin_prec = max(self.eps_bin_prec, other.eps_bin_prec) |
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 if you're adding one carefully-crafted rotation-containing GateCounts with a low eps_precision to a GateCounts that doesn't contain any rotations (it's just e.g. Ts and Toffolis) so it has the default eps_precision of 20
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.
Used np.format_scientific_float
to avoid such issues.
The code snippet I was trying to use (which works with this PR): import sympy
from qualtran.bloqs.basic_gates import ZPowGate
from qualtran.resource_counting import get_cost_value, QECGatesCost
t, eps = sympy.symbols(r"t \epsilon")
bloq = ZPowGate(exponent=t, eps=eps)
costs = get_cost_value(bloq, QECGatesCost())
costs.total_t_count() |
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 don't think this representation is particularly intuitive, so it needs solid documentation
eps_bin: FloatRepr_T = eps | ||
else: | ||
eps_bin = np.format_float_scientific(eps, precision=eps_repr_prec, unique=False) | ||
return cls(binned_rotation_epsilons=Counter({eps_bin: n_rotations})) |
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 still desperately needs a description of the data contained within the GateCounts
dataclass in the class docstring.
binned_rotation_epsilons=Counter( | ||
{eps_bin: other * n_rot for eps_bin, n_rot in self.binned_rotation_epsilons.items()} | ||
), |
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.
so how does it work when adding or multiplying when there are values that differ only in precision
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 __mul__
is to multiply with some int other
, right? which just multilpies the frequencies, not affecting the epsilons
.
For add, it just combines the two counters together, and equal keys will get merged. (handled by Counter.__add__
). The precision of representing epsilon is only used when converting it to a string key in from_rotation_with_eps
, but not stored in the GateCounts object itself.
if is_symbolic(eps): | ||
eps_bin: FloatRepr_T = eps | ||
else: | ||
eps_bin = np.format_float_scientific(eps, precision=eps_repr_prec, unique=False) |
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 unique=False
?
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 was to ensure that the representation was more stable. from the docs:
... If precision is given fewer digits than necessary can be printed. ...
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.
Actually, I'm not sure what the best way is to check if two rotation frequency tables are the same, as the epsilons could have been stored with different precisions, or be approximate.
Thinking more about this, I think we'll need a special data container for the binned rotations that enforces the invariants we want to enforce. It's too dangerous to have our public data member be a dictionary whose keys are supposed to be strings crafted in a very particular way. (There's a secondary issue that a dictionary is mutable and therefore not compatible with the frozen GateCounts class) |
fixes #1250
epsilons
for rotation bloqs.np.format_float_scientific
, with a default precision of10
digits.1.149 * log2(1.0 / eps) + 9.2
from https://arxiv.org/abs/1404.5320