-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add JSON serialization support for NoiseModel classes #7396
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
Conversation
@WingCode - thank you for working on this. The failing CI checks should give some pointers on what is amiss. Feel free to ask here if you need clarification. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7396 +/- ##
========================================
Coverage 98.68% 98.68%
========================================
Files 1091 1093 +2
Lines 96948 97109 +161
========================================
+ Hits 95674 95835 +161
Misses 1274 1274 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@pavoljuhas I have fixed the CI checks. |
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 code for serialization of contrib classes should be moved under cirq.contrib to limit any dependency of non-contrib cirq on cirq.contrib. Please see inline comments for other suggestions.
cirq-core/cirq/protocols/json_test_data/DampedReadoutNoiseModel.json
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/noise_models/noise_models.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/noise_models/noise_models.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/noise_models/noise_models.py
Outdated
Show resolved
Hide resolved
cirq-google/cirq_google/experimental/noise_models/noise_models_test.py
Outdated
Show resolved
Hide resolved
FYI that @pavoljuhas is out on jury duty (a mandatory civic obligation in the USA) starting today and possibly more days (depending on unpredictable factors). If he had commented on the PR or this issue before and hasn't responded, that's why. |
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 have not got through all files, but I think we can address the new comments before next iteration.
Co-authored-by: Pavol Juhas <[email protected]>
Co-authored-by: Pavol Juhas <[email protected]>
No need to change this interface to support JSON serialization.
Sync resolver cache with the contrib package `_class_resolver_dictionary` and Move previously serializable classes from `should_not_be_serialized` to `tested_elsewhere`. Also add copyright notice.
- do not change public attributes convert them to protected - use proper_repr for gate_durations_ns in repr - exclude optional arguments from repr when unspecified - change json schema to store class constructor arguments instead of object attributes - this allows to create instances in a standard way instead of using `cls.__new__`. - in json store heat, cool, dephase rates as values only to be zipped with qubits in the same order.
Already tested by the presence of .json and .repr files. Also check if optional rate arguments are skipped when unspecified.
- make the new noise-model class attributes read-only - remove unnecessary `_from_json_dict_` methods - test more json serde round trips over a list of instances
Drop unnecessary test of JSON serde and test more repr variants.
Keep sorted order as in main. There was no change otherwise.
- store unspecified probabilities as None in JSON - test serde and repr consistency of more instances in a list
- remove unnecessary methods `repr` and `_from_json_dict_` - test serde round-trip in noise_properties_test with a custom resolver - add NoiseModelFromNoiseProperties to cirq.json_resolver_cache
* Rewrite JSON support of NoiseModelFromNoiseProperties * Review PerQubitDepolarizingWithDampedReadoutNoiseModel * Revert cirq.protocols.json_test_data.spec * Keep sorted order in cirq.json_resolver_cache * Review noise_models_test * Review cirq.contrib.noise_models * Drop redundant test of ThermalNoiseModel serialization * Revisit ThermalNoiseModel serialization * Prefer `cache` to `lru_cache` for functions without arguments * Clean up cirq.contrib.json_test_data.spec * Restore cirq.contrib contents as it was before
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.
Sorry about long delay in reviewing. I have pushed in
several changes that addressed the remaining comments.
Thank you for contributing this!
This adds JSON serialization support and in most cases genuine
repr
support forAlso adds
cirq.contrib.json_test_data
for test data files specific to the classes incirq.contrib
module.Fixes #3846