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

Adding Test for Circuit Serialization #1176

Merged
merged 79 commits into from
Aug 14, 2023

Conversation

ItamarGoldman
Copy link
Contributor

@ItamarGoldman ItamarGoldman commented May 15, 2023

Summary

Adding circuit serialization tests to qiskit-experiments package as suggested in #1063

PR checklist

Add test to the following experiments:

  • Calibration experiments.
  • Characterization experiments.
  • QV experiment.
  • RB experiments.
  • Tomography experiments.

In addition, I fixed the the code of the following experiments:

  • Rabi Experiment - cast the amplitudes to float type after rounding so the the variable type will be serializable.
  • QV Experiment - casting the result (the probabilities) into a list so it will be serializable.

@wshanks
Copy link
Collaborator

wshanks commented May 17, 2023

@ItamarGoldman This looks good to me, but Chris made a suggestion here of using the json serializer rather than the qpy functions directly. I didn't update #1059 to follow that suggestion. Have you looked at that option?

@coruscating
Copy link
Collaborator

Another issue came up when I was testing #1183. QV circuits are currently not serializable when Aer isn't installed because Statevector.probabilities() returns a numpy array:

https://github.com/Qiskit/qiskit-experiments/blob/38678c283f1d2463792650a29492fb77f0170f26/qiskit_experiments/library/quantum_volume/qv_experiment.py#L157

This needs to be cast to a list and tests should be added that check both Aer and Aer-less circuits.

deleted the circuit test with 'qpy' lib as it is tested in the json serialization as mentioned in IS-1059
@ItamarGoldman
Copy link
Contributor Author

@ItamarGoldman This looks good to me, but Chris made a suggestion here of using the json serializer rather than the qpy functions directly. I didn't update #1059 to follow that suggestion. Have you looked at that option?

Thanks! I changed the serialization to be used with the json serialization method (I used 'assertRoundTripSerializable(exp)' ) I will add the tests to other experiments.

reorganize 'test_state_tomography.py'
Added a test that create an experiment and then serialize it.
deleted 'test_circuit_serialization' function from standard RB as the roundtrip serialization of the experiment is already checking serialization of the quantum circuit.
Added serialization tests for half angle Experiment and ExperimentData objects.
@ItamarGoldman ItamarGoldman changed the title [WIP] Adding Test for Circuit Serialization Adding Test for Circuit Serialization Jul 9, 2023
Copy link
Collaborator

@coruscating coruscating 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 working on this. Please write a release note describing the bug fixes for Rabi and QV experiments.

drag = RoughDrag([0], self.x_plus)
drag.set_experiment_options(reps=[2, 4, 8])
drag.backend = FakeWashingtonV2()
self.assertRoundTripSerializable(drag._transpiled_circuits())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are some tests on _transpiled_circuits() while others are on circuits()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I didn't see a reason to use _transpiled_circuits(), I tried to use the same method as the other tests used to be on the safe side but I don't think it is necessary (from my checks, there is no difference).
Should I change it to circuits() instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think _transpiled_circuits() makes more sense since those are the actual circuits being serialized and run on the backend, right? For most cases, there shouldn't be a difference but I think we should be consistent.

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 chose to use circuits() or _transpiled_circuits() by matching what method the other tests in the class are using. Should I change all the other to be _transpiled_circuits() or should I leave it as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's change them all to _transpiled_circuits(), then I think the PR should be ready to go.

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2023

CLA assistant check
All committers have signed the CLA.

github-merge-queue bot pushed a commit that referenced this pull request Jul 20, 2023
### Summary

This PR adds `requirements-extras.txt` which contains extra packages
that can be installed as `[extras]` and were mostly moved from
`requirements-dev.txt`. These are in contrast to the core dependencies
in `requirements.txt` and the requirements that are only used for tests,
which will stay in `requirements-dev.txt`.

### Details and comments

While the optional packages are not related to each other, it seemed too
complicated to split out optional dependencies into further categories.

There are a few additional changes:

- Added `qiskit-ibm-provider>=0.6.1` as an optional dependency since
this release adds numpy serialization, which fixes some known bugs with
circuit metadata serialization (see also #1176)
- Unpinned `scipy` (done in #1164) and bumped `cvxpy` to 1.3.2, which is
now compatible with scipy 1.11
- Removed matrix variables from CI workflow that doesn't use them
- Updated install and contributing instructions
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

Also +1 for Helena's comment to test _transpiled_circuits() instead of circuits()

test/library/calibration/test_fine_amplitude.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@coruscating coruscating left a comment

Choose a reason for hiding this comment

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

This looks good besides one minor comment. If @eliarbel also thinks it's good then it can be merged after that.

Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

ProcessTomography test should use _transpiled_circuits

test/library/tomography/test_process_tomography.py Outdated Show resolved Hide resolved
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Itamar

@coruscating coruscating added this pull request to the merge queue Aug 14, 2023
Merged via the queue into qiskit-community:main with commit c66034c Aug 14, 2023
nkanazawa1989 pushed a commit to nkanazawa1989/qiskit-experiments that referenced this pull request Jan 10, 2024
### Summary

Adding circuit serialization tests to qiskit-experiments package as
suggested in qiskit-community#1063

### PR checklist 

Add test to the following experiments:
- [x] Calibration experiments.
- [x] Characterization experiments.
- [x] QV experiment.
- [x] RB experiments.
- [x] Tomography experiments.

In addition, I fixed the the code of the following experiments:
- [x] Rabi Experiment - cast the amplitudes to `float` type after
rounding so the the variable type will be serializable.
- [x] QV Experiment - casting the result (the probabilities) into a list
so it will be serializable.

---------

Co-authored-by: Helena Zhang <Helena.Zhang@ibm.com>
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
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.

5 participants