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

Use rotation eps in GateCounts #1255

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

anurudhp
Copy link
Contributor

@anurudhp anurudhp commented Aug 6, 2024

fixes #1250

  • Maintain a frequency table of epsilons for rotation bloqs.
  • Epsilon is converted to a string (the dictionary key) using np.format_float_scientific, with a default precision of 10 digits.
  • T-costs:
    • Direct synthesis: 1.149 * log2(1.0 / eps) + 9.2 from https://arxiv.org/abs/1404.5320
    • Beverland et. al. model: passes the number of rotations, ignoring their epsilons.

@mpharrigan
Copy link
Collaborator

we had discussed binning by ceil(log(eps)) -- can you comment on that

@anurudhp
Copy link
Contributor Author

anurudhp commented Aug 7, 2024

@tanujkhattar's comment:

It's better to store frequency of eps by assuming we can represent float eps as a fraction x / 2^{precision} and store frequencies of x; with precision as a configurable parameter in the constructor of QECGateCount class. It's also fine to store frequency of eps directly for now and make this change as a future improvement. I'd advice against storing frequency of ceil(log2(1/eps)) since it's a very aggressive approximation.

Binning by the above integer approximation can be implemented.

My bigger concern is the various epsilons floating around:

  • bloq.eps for rotation bloqs
  • error_budget in the surface code costing functions

Should we remove this error_budget and only use bloq.eps? I am not familiar enough with the surface_code implementation to decide if this is correct.

@mpharrigan
Copy link
Collaborator

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 GateCounts. So, since the beverland physical cost model just counts "rotations" and gives one third of the total error budget to synthesizing them, the get_beverland_counts method should just return the number of rotations in the appropriate field. The gidney-fowler models consume get_total_t_and_ccz_counts, which can use the per-gate epsilon values to turn it into t and ccz counts

):
"""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
Copy link
Collaborator

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

Copy link
Contributor Author

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

@anurudhp anurudhp force-pushed the 2024/08/06-t-compl-use-rotation-eps branch from 3e542af to 0b7a9e3 Compare August 7, 2024 19:56
@anurudhp anurudhp requested a review from mpharrigan August 7, 2024 23:00
@anurudhp
Copy link
Contributor Author

anurudhp commented Aug 7, 2024

@mpharrigan ptal!

@anurudhp anurudhp changed the title [prototype] Use rotation eps in GateCounts Use rotation eps in GateCounts Aug 7, 2024
@anurudhp anurudhp force-pushed the 2024/08/06-t-compl-use-rotation-eps branch from 00a226b to 9b45a0b Compare August 8, 2024 00:22
@mpharrigan
Copy link
Collaborator

taking a look now. Can you update the original PR description with a summary of changes in this pr

Copy link
Collaborator

@mpharrigan mpharrigan left a 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

qualtran/resource_counting/_bloq_counts.py Outdated Show resolved Hide resolved
qualtran/resource_counting/_bloq_counts.py Outdated Show resolved Hide resolved
qualtran/resource_counting/_bloq_counts.py Outdated Show resolved Hide resolved
qualtran/resource_counting/_bloq_counts.py Outdated Show resolved Hide resolved
Comment on lines 139 to 142
binned_rotation_epsilons: Counter[int] = field(
factory=Counter, converter=_mapping_to_counter, eq=lambda d: tuple(d.items())
)
eps_bin_prec: int = 20
Copy link
Collaborator

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.

Copy link
Contributor Author

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)
Copy link
Collaborator

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

Copy link
Contributor Author

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.

@anurudhp
Copy link
Contributor Author

anurudhp commented Aug 8, 2024

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()

@anurudhp anurudhp requested a review from mpharrigan August 9, 2024 21:33
Copy link
Collaborator

@mpharrigan mpharrigan left a 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

qualtran/resource_counting/_bloq_counts.py Outdated Show resolved Hide resolved
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}))
Copy link
Collaborator

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.

Comment on lines +197 to +199
binned_rotation_epsilons=Counter(
{eps_bin: other * n_rot for eps_bin, n_rot in self.binned_rotation_epsilons.items()}
),
Copy link
Collaborator

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

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 __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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why unique=False?

Copy link
Contributor Author

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. ...

Copy link
Contributor Author

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.

@mpharrigan
Copy link
Collaborator

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)

@mpharrigan mpharrigan marked this pull request as draft October 15, 2024 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate rotation epsilon in gate cost
2 participants