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

Clifford simultaneous filtered rb #412

Closed
wants to merge 35 commits into from
Closed

Conversation

vodovozovaliza
Copy link
Contributor

@vodovozovaliza vodovozovaliza commented Jun 20, 2023

(Juan): This will need qiboteam/qibo#1015 to save and load circuits properly

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@vodovozovaliza vodovozovaliza self-assigned this Jun 20, 2023
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #412 (76f40c0) into main (e8774fd) will increase coverage by 0.12%.
Report is 50 commits behind head on main.
The diff coverage is 96.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
+ Coverage   95.90%   96.02%   +0.12%     
==========================================
  Files          83       85       +2     
  Lines        5740     5994     +254     
==========================================
+ Hits         5505     5756     +251     
- Misses        235      238       +3     
Flag Coverage Δ
unittests 96.02% <96.91%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/qibocal/protocols/characterization/__init__.py 100.00% <100.00%> (ø)
...s/characterization/randomized_benchmarking/data.py 100.00% <100.00%> (ø)
...haracterization/randomized_benchmarking/fitting.py 100.00% <100.00%> (+6.34%) ⬆️
...cterization/randomized_benchmarking/noisemodels.py 100.00% <100.00%> (ø)
...cterization/randomized_benchmarking/standard_rb.py 97.35% <100.00%> (-1.33%) ⬇️
...on/randomized_benchmarking/clifford_filtered_rb.py 99.29% <99.29%> (ø)
.../characterization/randomized_benchmarking/utils.py 98.64% <96.96%> (-1.36%) ⬇️
...s/characterization/randomized_benchmarking/plot.py 95.34% <95.34%> (ø)
...erization/randomized_benchmarking/circuit_tools.py 82.92% <66.66%> (+0.42%) ⬆️

... and 1 file with indirect coverage changes

Base automatically changed from liza/rb_bootstrap to main June 22, 2023 13:51
@andrea-pasquale
Copy link
Contributor

@vodovozovaliza @wilkensJ what is the status of this PR?

@andrea-pasquale andrea-pasquale added this to the Qibocal 0.0.5 milestone Aug 31, 2023
@andrea-pasquale
Copy link
Contributor

@ingoroth is someone going to work on this?

@ingoroth
Copy link
Contributor

At the moment nobody is actively working on this anymore. @vodovozovaliza might find some time to push this again at some point in the future. Maybe @Jacfomg has capacities to take over.

@Jacfomg
Copy link
Contributor

Jacfomg commented Sep 13, 2023

At the moment nobody is actively working on this anymore. @vodovozovaliza might find some time to push this again at some point in the future. Maybe @Jacfomg has capacities to take over.

We can talk about it next week if it's ok

@Jacfomg Jacfomg mentioned this pull request Sep 21, 2023
4 tasks
@andrea-pasquale
Copy link
Contributor

@Jacfomg feel free to merge main and update the qibo dep in pyproject.toml.

I believe that there is an issue with latest release of qibolab. I'm having troubles also here #539 :(

@Jacfomg
Copy link
Contributor

Jacfomg commented Oct 5, 2023

I think those tests should pass once we merge #499 that has the coupler changes needed.

@andrea-pasquale andrea-pasquale removed do not merge Soft warning to avoid merging a PR for reason provided in the PR qibo release required PR requires qibo feature not available in current poetry.lock labels Oct 11, 2023
Copy link
Contributor

@andrea-pasquale andrea-pasquale left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @Jacfomg.
A few more comments.
I tried to run the protocol using simulation
image
Perhaps it is better to move the legend under the plot with a horizontal layout.
Moreover, I'm seeing that in the matrix a color is shown as nan is it normal or am I doing something wrong?

@andrea-pasquale
Copy link
Contributor

@Jacfomg I believe that if we manage to run at least once on hardware without errors we should be ready to merge this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a quick comment here.
Even if the parameters/data/results are the same as the StandardRB could you please add a separate class for each just for readability?

@andrea-pasquale
Copy link
Contributor

@andrea-pasquale
Copy link
Contributor

Could you also fix conflicts?

@scarrazza scarrazza modified the milestones: Qibocal 0.0.5, Qibocal 0.0.6 Nov 2, 2023
@andrea-pasquale andrea-pasquale added the rb Randomized benchmarking label Nov 6, 2023
Copy link
Contributor

@Edoardo-Pedicillo Edoardo-Pedicillo left a comment

Choose a reason for hiding this comment

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

Documentation is missing for some functions; it would be great to add it.

Comment on lines +50 to +53
if isinstance(new_layer, Gate):
nqubits = max(new_layer.qubits) + 1
elif all(isinstance(gate, Gate) for gate in new_layer):
nqubits = max(max(gate.qubits) for gate in new_layer) + 1
Copy link
Contributor

@Edoardo-Pedicillo Edoardo-Pedicillo Nov 6, 2023

Choose a reason for hiding this comment

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

If new_layer is always an iterable, you can reduce it to one if.

Comment on lines +54 to +56
elif isinstance(new_layer, Circuit):
nqubits = new_layer.nqubits
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does layer_gen return list of Gates or Circuit? A list of Gates can be seen as a circuit and vice versa.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I think the original functionality of passing either a single Gate or a Circuit is sufficient. I guess the motivation was to be able to pass a layer of parallel single qubit gates. But this can also be cast to Circuit when passed.

Comment on lines +56 to +61
else:
raise_error(
TypeError,
f"layer_gen must return type Circuit or Gate, but it is type {type(new_layer)}.",
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else:
raise_error(
TypeError,
f"layer_gen must return type Circuit or Gate, but it is type {type(new_layer)}.",
)

Is there a particular reason for raising this error? We're all adults here; I'm guessing the code will crash if the wrong type is used.

if not isinstance(depth, int) or depth < 0:
raise_error(ValueError, f"Depth: {depth}, must be type int and >= 0.")

# Generate a layer to get nqubits.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the first part of this function, if you need to know the number of qubits, you can add it as an input.

Copy link
Contributor

Choose a reason for hiding this comment

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

In principle there is no need to pass nqubits as it is implicit in the output of layer_gen.__call__(). But I don't like that now the first call of the generator is not added to the circuit. This makes it harder when one actually has to reproduce the result of a generator, say by restoring the rng state.

Comment on lines +184 to +188
if platform and platform.name != "dummy":
raise_error(
NotImplementedError,
f"Backend qibolab ({platform}) does not perform noise models simulation.",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we skip this if and set directly the numpy backend?

# Compute fitting uncertainties
if len(popt_estimates):
perr = data_uncertainties(popt_estimates, uncertainties, data_median=popt)
perr = perr.T if perr is not None else (0,) * len(popt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
perr = perr.T if perr is not None else (0,) * len(popt)
perr = perr.T if perr else (0,) * len(popt)

Comment on lines +332 to +334
# TODO: If fit is None: loop to separate plotting for fitting in qq

# TODO: fit doesn't get loaded on acquisition tests
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of TODOS, I suggest to collect them in an issue.

Comment on lines +403 to +416
# TODO: This mess as well ...
# meta_data = deepcopy(data.attrs)
# meta_data.pop("depths", None)
# if not meta_data["noise_model"]:
# meta_data.pop("noise_model")
# meta_data.pop("noise_params")
# elif meta_data.get("noise_params", None) is not None:
# meta_data["noise_params"] = np.round(meta_data["noise_params"], 3)

# table_str = "".join(
# [f" | {key}: {value}<br>" for key, value in {**meta_data, **rb_params}.items()]
# )

# table_str = table_html(table_dict("Something"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code, delete it

Comment on lines +36 to +47
if gate["name"] == "rz":
gatelist.append(
gates.RZ(gate["_target_qubits"][0], gate["init_kwargs"]["theta"])
)
if gate["name"] == "rx":
gatelist.append(
gates.RX(gate["_target_qubits"][0], gate["init_kwargs"]["theta"])
)
if gate["name"] == "ry":
gatelist.append(
gates.RY(gate["_target_qubits"][0], gate["init_kwargs"]["theta"])
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This lines can be collected in a for loop using getattr

Comment on lines +48 to +53
if gate["name"] == "z":
gatelist.append(gates.Z(gate["_target_qubits"][0]))
if gate["name"] == "x":
gatelist.append(gates.X(gate["_target_qubits"][0]))
if gate["name"] == "y":
gatelist.append(gates.Y(gate["_target_qubits"][0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

also this one

@Edoardo-Pedicillo
Copy link
Contributor

Edoardo-Pedicillo commented Mar 29, 2024

This PR is outdated, and it seems it will not be concluded, btw the protocol is interesting so I have opened #779

@andrea-pasquale andrea-pasquale deleted the clifford_filtered_rb branch June 10, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy on hardware rb Randomized benchmarking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants