-
Notifications
You must be signed in to change notification settings - Fork 14
feat: implement error-budgeting #76
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
base: main
Are you sure you want to change the base?
Conversation
rolandriver
left a comment
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.
Thanks @nelimee. Quite a fair amount of comments as I think we should define data structures that'll help simplify the whole computation and reduce the number of passed arguments. Happy to discuss this in detail.
| ) | ||
|
|
||
|
|
||
| def get_error_budget( |
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 would strongly advise to avoid functions with more than 3 or 4 parameters. Having 13 parameters here is practically unusable. At first glance, a lot of them can be grouped under dedicated custom datatypes. For instance, noise_model_parameters is present in NoiseInterface. We should think of passing instances instead of types here I think.
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 with this - I think a lot of parameters can be nicely encapsulated into datatypes. Also, does this function need to be able to generate the circuits or can we pass the circuits to it? I'm thinking a dictionary of {distance: circuit}. You can apply different noises to this circuit once it is generated. Ideally, you also extract the number of rounds from the circuit (e.g. from detector coordinates).
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.
It is unclear to me what will be gained by that. To me, this is a 4-parameter function for the average user, and the power user is given optional knobs to turn, approximately like a sinter.collect would do.
Also, in my opinion, it would be even more cumbersome for a user to use data types just for grouping parameters, because then he/she needs:
- to find which data types (easy with type hints),
- import these data types,
- create an instance of these data types and give them to the function.
For the circuit part, a callable is a strictly more capable way of doing. If you have a dictionary of {distance: {num_round: circuit}}, you can just wrap it in a function that will return the entry if it exists, or fail with a meaningful error message if the circuit required is not in the mapping.
But I am happy to also allow a dictionary if you think that is a nice-to-have.
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 as soon as you expose parameters publicly, it means they can be used. Defaulting them doesn't mean that a user can't try and fiddle with them. Even a power user wouldn't be able to humanly understand what are the possiblilties of tweaking 10 parameters, least to remember what they are supposed to do. If these parameters are to be understood as experiment configuration, then they should fall under a datastructure that represents that concept.
| """ | ||
| # We will compute the gradient at the half point following the methodology outlined | ||
| # in "Exponential suppression of bit or phase errors with cyclic error correction". | ||
| point = np.asarray(noise_model_parameters) / 2 |
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.
| point = np.asarray(noise_model_parameters) / 2 | |
| point = np.asarray(noise_model_parameters) / 2. |
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.
Does that change anything? Or is that just a personal stylistic preference?
deltakit-explorer/src/deltakit_explorer/analysis/budget/_budget.py
Outdated
Show resolved
Hide resolved
| # in "Exponential suppression of bit or phase errors with cyclic error correction". | ||
| point = np.asarray(noise_model_parameters) / 2 | ||
| # Evaluate the gradient. | ||
| gradient, gradient_stddev = compute_1_over_lambda_gradient_at( |
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.
Same, it is really hard for a user to use such functions, especially when they are exposed publicly.
deltakit-explorer/src/deltakit_explorer/analysis/budget/_generation.py
Outdated
Show resolved
Hide resolved
| derivative = float( | ||
| sum( | ||
| coefficient * (power + 1) * gradient_approximation_point**power | ||
| for power, coefficient in enumerate(coefficients[1:]) |
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 excluding the first element here ?
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.
Because in the derivative, the constant coefficient (associated with x**0, so the first one) is not needed.
deltakit-explorer/src/deltakit_explorer/analysis/budget/_gradient.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| def compute_1_over_lambda_gradient_at( | ||
| noise_model_type: type[NoiseInterface], |
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.
Same: this signature should be simplified. It is unusable in state.
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 6673bb8 |
|
Thanks for including me, happy to have a look at this. |
Skoricius
left a comment
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.
Thanks for this - it's going to be very useful for the project work.
For now, mainly commenting on the interface. Will look more closely into plotting and maths tomorrow.
| ) | ||
|
|
||
|
|
||
| def get_error_budget( |
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 with this - I think a lot of parameters can be nicely encapsulated into datatypes. Also, does this function need to be able to generate the circuits or can we pass the circuits to it? I'm thinking a dictionary of {distance: circuit}. You can apply different noises to this circuit once it is generated. Ideally, you also extract the number of rounds from the circuit (e.g. from detector coordinates).
| noise_model_type: type[NoiseInterface], | ||
| noise_model_parameters: npt.NDArray[np.floating] | Sequence[float], | ||
| num_rounds_by_distances: Mapping[int, Sequence[int]], | ||
| noise_parameters_exploration_bounds: list[tuple[float, float]], |
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 needs to be a mandatory input because it's a bit hard for the user to determine what's sensible here. I'd say this can be optional and should be some relative to the size of the parameters. E.g. default to 30% or something like that?
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.
That's a part where I spent a lot of time trying. The problem is that because some parameters are way more sensitive than others (e.g., 2-qubit gate noise is more sensitive than 1-qubit gate noise), I was not able to find a fixed rule that would be fine for all the parameters. Either the bounds are too tight and estimation of the gradient on non-sensitive parameters (e.g., 1-qubit gate noise) are bad, or the bounds are too large and we end up with LEP that are at 50% for some variations and at 0% (i.e., no error witnessed) for others. Happy to re-test if you want.
deltakit-explorer/src/deltakit_explorer/analysis/budget/_budget.py
Outdated
Show resolved
Hide resolved
deltakit-explorer/src/deltakit_explorer/analysis/budget/_budget.py
Outdated
Show resolved
Hide resolved
deltakit-explorer/src/deltakit_explorer/analysis/budget/_budget.py
Outdated
Show resolved
Hide resolved
deltakit-explorer/src/deltakit_explorer/analysis/budget/_discretisation.py
Show resolved
Hide resolved
| """ | ||
| circuit = memory_generator(distance, num_rounds) | ||
| noisy_circuit = noise_model.apply(circuit) | ||
| decoder, decoder_circuit = PyMatchingDecoder.construct_decoder_and_stim_circuit( |
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 would imagine it would be important to select the decoder we are using for lambda modelling.
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.
Yes it is, but for the moment at least there is no interface in deltakit for the construction of an arbitrary decoder. Said differently, deltakit_decode.DecoderProtocol does not include in its interface any generic way to construct a decoder.
Because I need to construct the decoder for various circuits and noise, I need such a capability. And as far as I have seen, PyMatchingDecoder is the only offline decoder that provides that for the moment.
AndrewPattersonRL
left a comment
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.
Looks good - I've ran the notebook too.
The main takeaway from me is that it could save time if you generate noiseless circuits first, then apply noise to each one. Happy to chat about that if you like
deltakit-explorer/src/deltakit_explorer/analysis/budget/__init__.py
Outdated
Show resolved
Hide resolved
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 think budget is a bit too vague of a name for a module - error_budgeting?
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 find error_budgeting a little bit long. Maybe error_budget?
deltakit-explorer/tests/analysis/budget/test_post_processing.py
Outdated
Show resolved
Hide resolved
| "output_type": "display_data" | ||
| } | ||
| ], | ||
| "source": [ |
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.
First time I ran this cell I got a warning that no fail had been found, then an error that my data was of the incorrect format. I presume this is because the no fails found entry had been removed. Should we make the lower bounds a bit larger so that this is very unlikely to happen?
(stupidly I re-ran the cell without saving the error message so I can't repeat it here)
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 think it's useful - in your experience does it generally produce round numbers that are greater or less than the distance? Or is that noise-dependent?
rolandriver
left a comment
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.
Same as before. I really think these functions need to be refactored into smtg more useable. In the interest of time, I approve but this shouldbe changed quickly.
| ) | ||
|
|
||
|
|
||
| def get_error_budget( |
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 as soon as you expose parameters publicly, it means they can be used. Defaulting them doesn't mean that a user can't try and fiddle with them. Even a power user wouldn't be able to humanly understand what are the possiblilties of tweaking 10 parameters, least to remember what they are supposed to do. If these parameters are to be understood as experiment configuration, then they should fall under a datastructure that represents that concept.
| def __call__( | ||
| self, a: float, b: float, c: float, num_points: int, degree: int, / | ||
| ) -> npt.NDArray[np.floating]: | ||
| match self: |
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 not supported in Python 3.10
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 match? It seems like it is: https://peps.python.org/pep-0636/. Also, tests are passing for 3.10 so I think this is fine.
🔗 Closed Issues
Closes #66.
📝 Description
This PR implements error budgeting. A lot of code in this PR is about:
🚦 Status
🛠️ Future Work
None
➕️ Additional Information
None
🧾 Release Note
PR title