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

Serialization of measurements #1431

Merged
merged 9 commits into from
Sep 4, 2024
Merged

Conversation

BrunoLiegiBastonLiegi
Copy link
Contributor

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi commented Aug 29, 2024

This addresses the bug #1426 by implementing a proper serialization of measurement gates and their result.
Furthermore, the possibility to specify the basis of measurement gates with strings (for instance "Z" or ["X", "Z", "Y"]) has been added.
I also made some minor modifications to MeasurementResult, and I would also like to remove the circuit attribute from it. At the moment it is only used to generate the samples when they are not available. However, as I see it, the MeasurementResult should always be associated with the samples.

Checklist:

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

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (21732ad) to head (7864590).
Report is 123 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1431   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          78       78           
  Lines       11317    11324    +7     
=======================================
+ Hits        11311    11318    +7     
  Misses          6        6           
Flag Coverage Δ
unittests 99.94% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BrunoLiegiBastonLiegi BrunoLiegiBastonLiegi marked this pull request as ready for review August 30, 2024 13:00
Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

Some suggestions, but they are not incredibly relevant, as the whole measurement storage is a bit convoluted (with many references everywhere), and a greater action should be taken to simplify it (not now, possibly as part of qibo-core).

The relevant part instead it's that a test is missing. It would be nice to have a test failing before this PR, and now working.

src/qibo/measurements.py Show resolved Hide resolved
src/qibo/measurements.py Outdated Show resolved Hide resolved
src/qibo/gates/measurements.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

alecandido commented Aug 30, 2024

Furthermore, the possibility to specify the basis of measurement gates with strings (for instance "Z" or ["X", "Z", "Y"]) has been added.

Try to keep features' implementation as separate as possible (and separate from bug fixes).

(Ok, this is really a tiny one, but it was worth to mention the general rule)

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Yeah, let's say that rather than a new feature is a side effect of the fix in this case, but I fundamentally agree.

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

Ok, I should have addressed everything now, except the MeasurementResult.circuit attribute that is still there, but I would get rid of it if everyone agrees.

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

The test is effective and passing.

Thanks @BrunoLiegiBastonLiegi!

@marcorossi5
Copy link

Hello guys, I've installed qibo from this source branch and checked if the qibo-webappdaemon works smoothly.
Unfortunately we are having the following error raised by the following snippet:

with open(circuit_path, "r") as f:
      raw = json.load(f)
c = qibo.Circuit.from_dict(raw)
r = c(nshots=settings.get("nshots"))
print(f"Results: {r.samples()}")
Traceback (most recent call last):
  File "/usr/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/nfs/users/tiiq/webappdaemon/src/daemon/slurm_circuit_computation.py", line 62, in <module>
    main(args.job_folder)
  File "/nfs/users/tiiq/webappdaemon/src/daemon/slurm_circuit_computation.py", line 55, in main
    print(f"Results: {r.samples()}")
  File "/nfs/users/tiiq/qibo/src/qibo/result.py", line 324, in samples
    qubits = self.measurement_gate.target_qubits
AttributeError: 'NoneType' object has no attribute 'target_qubits'

@scarrazza
Copy link
Member

Do you have a simpler example using only qibo primitives?

@marcorossi5
Copy link

marcorossi5 commented Sep 4, 2024

I did a little bit of investigation. It seems that the circuit serialization done with circuit.raw looks different when launched on a CPU only queue (like sim) or on a QPU one (qw11q).

The script is the following:

import json
import qibo

c = qibo.Circuit(5)
c.add(qibo.gates.M(0))

with open("circuit.json", "w") as f:
    json.dump(c.raw, f)

print(c.raw)

Here you have the results:

Output on sim

{'queue': [{'name': 'measure', 'init_args': (0,), 'init_kwargs': {'register_name': None, 'collapse': False, 'basis': ['Z'], 'p0': None, 'p1': None}, '_target_qubits': (0,), '_control_qubits': (), '_class': 'M', 'measurement_result': {'samples': None}}], 'nqubits': 5, 'density_matrix': False, 'qibo_version': '0.2.12'}

Output on qw11q:

{'queue': [{'name': 'measure', 'init_args': [0], 'init_kwargs': {}, '_target_qubits': [0], '_control_qubits': [], '_class': 'M'}], 'nqubits': 5, 'density_matrix': False, 'qibo_version': '0.2.11'}

It seems that there's a mismatch when loading a circuit on the QPU that was created on CPU and viceversa.

The code that raises the following errors is:

import json
import qibo
with open("circuit.json") as f:
    raw = json.load(f)

c = qibo.Circuit.from_dict(raw)

If I create the circuit on CPU => load it on QPU:

Traceback (most recent call last):
  File "/nfs/users/tiiq/from_raw.py", line 6, in <module>
    c = qibo.Circuit.from_dict(raw)
  File "/nfs/tools/qibo/lib/python3.10/site-packages/qibo/models/circuit.py", line 1137, in from_dict
    circ.add(Gate.from_dict(gate))
  File "/nfs/tools/qibo/lib/python3.10/site-packages/qibo/gates/abstract.py", line 109, in from_dict
    gate = cls(*raw["init_args"], **raw["init_kwargs"])
  File "/nfs/tools/qibo/lib/python3.10/site-packages/qibo/gates/measurements.py", line 101, in __init__
    gate = basis_cls(qubit).basis_rotation()
TypeError: 'str' object is not callable
srun: error: fahid: task 0: Exited with exit code 1

If I create the circuit on QPU => load it on CPU:

Traceback (most recent call last):
  File "/nfs/users/tiiq/from_raw.py", line 6, in <module>
    c = qibo.Circuit.from_dict(raw)
  File "/nfs/users/tiiq/qibo/src/qibo/models/circuit.py", line 1137, in from_dict
    circ.add(Gate.from_dict(gate))
  File "/nfs/users/tiiq/qibo/src/qibo/gates/abstract.py", line 121, in from_dict
    if raw["_class"] == "M" and raw["measurement_result"]["samples"] is not None:
KeyError: 'measurement_result'

@BrunoLiegiBastonLiegi
Copy link
Contributor Author

BrunoLiegiBastonLiegi commented Sep 4, 2024

The output on sim should be the correct one, which was updated in this PR. The other one seems to be the old serialization of the M gate (the one in main).

Similarly below, the CPU -> QPU loading seems to fail because the M object is not able to receive strings as basis arguments, which is an additional feature introduced here. Viceversa, the QPU -> CPU loading fails because the object serialized on the QPU doesn't include attributes that are required to load a measurement now, i.e. the serialization of the MeasurementResult.

I would say that the CPU and QPU are using two different versions of qibo:

  • CPU -> this branch
  • QPU -> main

@marcorossi5
Copy link

Let me check again the environments. If the branch mismatch is the cause, then we are fine.

@scarrazza
Copy link
Member

I have just tried your code and it works, no crashes.

@marcorossi5
Copy link

Yes, I agree. I was loading the wrong environment on the QPU, my bad.
I guess the test is passed then

@scarrazza scarrazza merged commit 4f2314a into master Sep 4, 2024
27 checks passed
@renatomello renatomello deleted the measurement_result_serialization branch October 17, 2024 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants