Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

[WIP] Generate json files for tests (which used pickled data) #398

Closed
wants to merge 14 commits into from

Conversation

dekool
Copy link
Contributor

@dekool dekool commented Apr 28, 2020

Summary

changed the pickle files used in the fitter tests into json files.
in addition, reordered the comparing of the real results and the expected results into one function.

Details and comments

  • in order to create the json files I loaded the pickle files and saved the data inside them as json files (using the "save_results_as_json" function I added in the utils).
  • in addition, I didn't changed the tests results, just splited between the tests settings and expected results for easier change of the way the expected results are kept (I will save them also in a json format)
  • I will add a code that generate the expected results and saves them as a json

I also checked the sizes of the json files compared to the pickle files, and found that -

  • for small files (less than 100KB) - the json files are larger in around 2KB
  • for large files - the json files are a bit smaller than the pickle files

the run times of the tests are almost the same with pickle files and json files (when running them on my laptop) - with the json files the average was a little faster (in the scale of only 1/100 sec)

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2020

CLA assistant check
All committers have signed the CLA.

@chriseclectic
Copy link
Collaborator

Hi @dekool thanks for starting this. Can you have a look at the discussion and final merged version of PR #387 for how to add a script for re-generating the reference JSON files.

@ShellyGarion
Copy link
Contributor

It will be better to merge this only after #407 is merged.
So that the json files with the results will be generated using the new Clifford / CNOTDihedral classes.
In addition, there will be an option to add a random seed in randomized_benchmarking_seq.

@chriseclectic
Copy link
Collaborator

@ShellyGarion if you do that can we integrate this PR with those so we can test them. Currently several tests are disabled for RB (#402) so we can't evaluate that those PRs are working correctly.

@chriseclectic chriseclectic marked this pull request as ready for review May 19, 2020 14:14
@dekool dekool changed the title [WIP] Replace pickle files to json in rb tests [WIP] Generate json files for tests (which used pickled data) Jun 2, 2020
coherent_results.append(qiskit.execute(current_circ, backend=backend,
basis_gates=basis_gates,
shots=shots,
noise_model=noise_model,
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be coherent_noise_model?


The simulation results file will contain a list of Result objects in a dictionary format.
The fitter data file will contain dictionary with the following keys:
- 'cnotdihedral_Z_ydata', value is stored in the form of list of dictionaries with keys of:
Copy link
Contributor

Choose a reason for hiding this comment

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

cnot_dihedral does not belong here...

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you mean 'original_ydata' ?

@ShellyGarion
Copy link
Contributor

Note: the generation of RB json files depends on #407

Hence the code in this PR should be revised after #407 is merged.

  1. The Cliffords functions for random / decompose were updated
  2. There should be a fixed seed for generating the RB sequences
  3. The API for interleaved RB was updated

Copy link
Contributor

@yaelbh yaelbh left a comment

Choose a reason for hiding this comment

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

  1. Is the code in the generate_data.py files a duplicate of code that lies somewhere else? If yes then the duplication should be removed. If no then how did you decide how to write it?
  2. I recommend to split to three PRs, for measurement calibration, quantum volume, and RB. It's ok to have the same change in utils.py in all three PRs. This way only the RB pull request will depend on another non-merged RB pull request.

@dekool
Copy link
Contributor Author

dekool commented Jun 28, 2020

  1. most of the code is inspired by different qiskit notebooks explaining the relevant subjects. I can't call functions of the notebooks from the code, so I can't remove the duplicated parts easily. there are also some places which I used non-noisy tests of the same functionality as basis. at these places I can remove the duplication from the common part, but it will cause dependency between different tests
  2. good idea. I will split this PR into 3

@ShellyGarion
Copy link
Contributor

Generally, you can also follow this example:
https://github.com/Qiskit/qiskit-ignis/blob/master/test/accreditation/generate_data.py

@dekool
Copy link
Contributor Author

dekool commented Jun 28, 2020

this PR was split into 3 PR in order to separate data generation of different functionalities -
RB - #444
QV - #445
measurement calibration - #446

@dekool dekool closed this Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants