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

Add getting started docs #72

Merged
merged 16 commits into from
Apr 2, 2020
Merged

Add getting started docs #72

merged 16 commits into from
Apr 2, 2020

Conversation

willzeng
Copy link
Contributor

Fixes #67

This PR includes a first minimal example of getting started. This first example uses cirq and in a future issue (#71) a qiskit example will be added. See that issue for a blocking question.

The PR includes:

  • sphinx documentation is added
  • tests for the code in the sphinx docs are added in a new test_zne.py module. This module introduces the first mitiq testing for using cirq front and back ends
  • introduction of default behavior in zne.py to make user interaction smoother when user is using cirq
  • some other small docs changes

The mitigation examples themselves come out very smoothly. However, it is unfortunate that there's a hunk of boilerplate needed because cirq's noise simulation functions don't have a great built-in interface. Suggestions on how best to remove this boilerplate would be appreciated.

An alternative option is to import these functions from the mitiq library directly however I worry that this would obscure what mitiq is actually doing. I wouldn't want folks to think that under the hood mitiq does weird things with noise simulation backends etc. It is good to be transparent about the separation between user space cirq code and mitiq calls.

mitiq/tests/test_zne.py Outdated Show resolved Hide resolved
mitiq/tests/test_zne.py Outdated Show resolved Hide resolved
mitiq/tests/test_zne.py Outdated Show resolved Hide resolved
mitiq/tests/test_zne.py Outdated Show resolved Hide resolved
# TODO this assumes is qiskit
scale_noise = qs_utils.scale_noise
if isinstance(qp, QuantumCircuit):
scale_noise = qs_utils.scale_noise
Copy link
Contributor

Choose a reason for hiding this comment

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

I think qs_utils.scale_noise modifies the base noise level, correct? If so, this should be changed to

(i) Convert from a Qiskit circuit to a Cirq circuit.
(ii) Use the input folding (scale noise) method.

I think using qs.scale_noise is fine for this PR, but this is one example where we need to be careful about conversions throughout the pipeline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that is right. We'll want to resolve this in #71

Copy link
Contributor

@rmlarose rmlarose left a comment

Choose a reason for hiding this comment

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

Looks great @willzeng! Some mostly minor comments to address then ready to merge.

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

I think it's great, thank you @willzeng!

  • I understand that in the Getting Started we want to get started as quickly as possible :), I was just wondering, re-reading the file, whether some of the theoretical framework that is here given for granted could be acknowledged, i.e., that noise enters the picture of unitary evolution of a quantum system, that quantum circuits assume this unitary evolution, that error mitigation implements post-processing methods to reduce this, etc. So this is maybe just a side note to ask/think about where should we put the more theoretical information in the guide structure.

  • I have a suggestion for meas_observable: since in the getting started we want to go to the application of mitiq functions as quickly as possible, without other stuff in the way, we could cut it from there. Actually I think that such function will come quite useful in the future: we could place it in the code base; the most apt place seems matrices.py, maybe, if needed, we could rename matrices.py something like numpy_utils.py or something like operations.py to be as general as possible.

  • Besides the previous comment, I propose to rename meas_observable more pythonic as measure_observable. It is a bit long, so maybe expect_observable or expect could work too? On the "observable" (relevant only if you think it is useful that this becomes a global function): could it be more generally defined on an operator, i.e., that is not Hermitian? Maybe in that case the obs_delta return would not be well defined?

  • I tested the code locally in a Jupyter notebook and it run fine. Just some functions for me need to be imported from mitiq.zne as suggested in the code.

  • I noticed that I already included in my PR doctest as a Sphinx extention. I will later fork this PR and build locally to check it, but it should be throwing warnings about it.

  • For me, the noise was not reduced. It printed: "Mitigation provides a 1.0factor of improvement.".
    and "Mitigated error with the linear methodis 0.0506".
    Two more general comments on very minor things:

  • single ticks around words, "``", will display as italics and not code-like in the docs. It may depend on the extension used in Sphinx, but with the most common ones and the current that's the case. We need double ticks "````" for that.

  • Maybe we need to think whether to use Mitiq or mitiq as branding?

docs/source/guide/guide-getting-started.rst Show resolved Hide resolved
docs/source/guide/guide-getting-started.rst Show resolved Hide resolved
docs/source/guide/guide-getting-started.rst Outdated Show resolved Hide resolved
docs/source/guide/guide-getting-started.rst Outdated Show resolved Hide resolved
@nathanshammah nathanshammah added this to In progress in Documentation, testing, release, prototyping via automation Mar 30, 2020
@nathanshammah nathanshammah added the documentation Improvements or additions to documentation label Mar 30, 2020
@willzeng
Copy link
Contributor Author

Thanks @nathanshammah and @rmlarose for the comments. I've made most changes.

Replies to Nathan's questions:

  1. Educational content about quantum noise and quantum computing will likely be most efficiently done by linking to other sources and tutorials. It could be good in future to add a future section for this. Captured this in Add background resource links to getting started #74 where more detail can be added later.

  2. I'd still worry that even if it was in the library, that having meas_observable as an import might obscure its functioning. For the getting started to be most useful IMO, it should just use base functions and external library functions except where mitiq is actually doing its work.

  3. I wasn't able to replicate the import errors (see above). There was a change to put those imports in the __init__.py. Are you still getting the same errors?

  4. Thanks for the `` hint.

  5. My vote is for mitiq lowercase :) Like when you import the package

  6. The noise not being reduced would definitely be a problem. Are the tests passing for you locally? The tests basically run the same code as in the notebook.

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

LGTM.

  • A reference to Richardson extrapolation may be helpful.
    Additional references could be added. I can check how to add a References section to the documentation.
  • The links to html need to be to .rst files I think, since with the current gitignore those are empty in here?

docs/source/guide/guide-getting-started.rst Show resolved Hide resolved
docs/source/guide/guide-getting-started.rst Show resolved Hide resolved
Documentation, testing, release, prototyping automation moved this from In progress to Review in progress Mar 31, 2020
@willzeng
Copy link
Contributor Author

@rmlarose and @nathanshammah Changes made. This is ready for your re-review. If it looks ready please do mark approval and then merge.

@willzeng
Copy link
Contributor Author

Strangely the github actions CI is not behaving as expected. The failing tests here are failing because of the flake8 problem on master (see #76 ) not because of code on this branch.

The tests that pass are just testing this branch, which does pass.

It isn't clear to me why the pull_request trigger is testing the master branch. I'll open an issue for discussion.

@andreamari
Copy link
Member

I noticed that in the getting started we never mention how mitiq scales noise to perform the extrapolation. We actually never say that noise is scaled at all.
Maybe this is ok for the getting started. I just wanted to be sure that we are aware of this.

Otherwise it looks good to me! Thanks @willzeng

@nathanshammah
Copy link
Member

It looks great. I can test it locally once in master to re-build the documentation pdf. I can see how to add references in a more general way. So that if we refer to the same paper in different points, it is a single reference section and not in each file.

The lint issue in CI is sorted out by #76, I introduced that error in #64.

@willzeng
Copy link
Contributor Author

@andreamari You're right and I agree. Getting started treats mitiq as a black box to keep things simple. Other sections and examples should detail how it actually works for those who want to know :)

willzeng and others added 10 commits March 31, 2020 15:33
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Nathan Shammah <nathan.shammah@gmail.com>
@willzeng
Copy link
Contributor Author

Rebased on top of master to pass tests.

@rmlarose rmlarose self-requested a review April 2, 2020 01:09
mitiq/tests/test_zne.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rmlarose rmlarose left a comment

Choose a reason for hiding this comment

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

One very minor suggestion (in two places) + typo fix. Looks great @willzeng!

Documentation, testing, release, prototyping automation moved this from Review in progress to Reviewer approved Apr 2, 2020
willzeng and others added 3 commits April 1, 2020 22:07
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
Co-Authored-By: Ryan LaRose <rlarose@umich.edu>
@willzeng willzeng merged commit c802ad0 into master Apr 2, 2020
Documentation, testing, release, prototyping automation moved this from Reviewer approved to Done Apr 2, 2020
@willzeng willzeng deleted the getting-started branch July 3, 2020 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add Getting Started: minimal example of ZNE (with any frontend/backend)
4 participants