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

Noise Scaling for LRE #2347

Merged
merged 22 commits into from
Jul 1, 2024
Merged

Noise Scaling for LRE #2347

merged 22 commits into from
Jul 1, 2024

Conversation

purva-thakre
Copy link
Collaborator

Fixes #2307

Description


License

  • I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

Copy link

codecov bot commented May 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.34%. Comparing base (b58f029) to head (bfe07bd).
Report is 73 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2347      +/-   ##
==========================================
+ Coverage   98.22%   98.34%   +0.11%     
==========================================
  Files          87       89       +2     
  Lines        4056     4105      +49     
==========================================
+ Hits         3984     4037      +53     
+ Misses         72       68       -4     

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

@purva-thakre
Copy link
Collaborator Author

@cosenal @vprusso This is ready for a review up to using layerwise folding on cirq circuits.

I am working on figuring out why layerwise folding on a pyquil circuit raises errors. Once this is fixed, laywerise folding for non-cirq circuits will be added.

mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved

def _get_chunks(
input_circuit: Circuit, num_chunks: Optional[int] = None
) -> List[Circuit]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: Are we still supporting versions of Python that wouldn't allow -> list[Circuit] vs. List[Circuit]?

Copy link
Collaborator Author

@purva-thakre purva-thakre Jun 10, 2024

Choose a reason for hiding this comment

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

This is a good question. Which versions of python did you have in mind? I know there's an open issue to drop 3.9 once we add support for 3.12.

The validate workflow only runs for python 3.11 which is used for type checking by mypy.

python-version: "3.11"
- name: Install Python dependencies
run: |
python -m pip install --upgrade pip
make install requirements
- name: Check types with mypy
run: make check-types

From what I could find, 3.9 uses list[Circuit] as a type alias and List[Circuit] for type annotations. Same with 3.10.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that type aliases List and Dict are deprecated starting from Python 3.9 https://docs.python.org/3/library/typing.html#deprecated-aliases

mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/zne/scaling/layer_scaling.py Outdated Show resolved Hide resolved
@purva-thakre
Copy link
Collaborator Author

I think it's better to limit this PR to Cirq circuits only because I need to spend some more time understanding how the Mitiq interface decorators work. I have used the default args for the decorators in the past without any issues. This PR requires using the optional arguments.

Two different ways to implement the optional args led to errors. Either the one_to_many argument was ignored (first code block) or _pop_measurement was used on a Qiskit circuit instead of converting the circuit to a cirq circuit first (second code block).

@accept_qprogram_and_validate
def multivariate_layer_scaling( input_circuit, degree, ............, one_to_many = True)

Similar to how the optional arg is used in PEC, I also tried changing multivariate_layer_scaling to _cirq_multivariate_layer_scaling such that the new multivariate_layer_scaling only accepted a QPROGRAM as input.

 noise_scaling_func = accept_qprogram_and_validate(
      _cirq_multivariate_layer_scaling,
      one_to_many=True,
  )
  scaled_circuits = noise_scaling_func(
      input_circuit, degree, fold_multiplier, num_chunks, folding_method
  )
 return scaled_circuits

@jordandsullivan
Copy link
Contributor

Sounds good Purva, we can limit this PR for Cirq for now.

@purva-thakre purva-thakre marked this pull request as ready for review June 21, 2024 22:59
@purva-thakre purva-thakre requested review from cosenal and vprusso June 21, 2024 23:01
@purva-thakre
Copy link
Collaborator Author

Except for 1 unresolved comment #2347 (comment), this PR is ready for another review.


def _get_chunks(
input_circuit: Circuit, num_chunks: Optional[int] = None
) -> List[Circuit]:
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL that type aliases List and Dict are deprecated starting from Python 3.9 https://docs.python.org/3/library/typing.html#deprecated-aliases

terminal measurements
"""

_check_foldable(input_circuit)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can raise an UnfoldableCircuitError, it should be documented in the docstring of this caller function as well.

Copy link
Collaborator Author

@purva-thakre purva-thakre Jun 22, 2024

Choose a reason for hiding this comment

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

I had included it previously. #2347 (comment)

We should agree on style guidelines for this project and update the docs style guidelines section on docstrings.

Couple of things that confused me:

  • Since I am relying on private functions, a user won't be able to read the source of the error in the API. Which is why I felt it's better to document the raised errors in the public function.
  • Referring to Vincent's comment on raising errors from a function in a different module, the API-doc page only mentions UnfoldableCircuitError once as a defined exception. Since _check_foldable is a private function, the error raised by the function does not show up publicly.
  • Most of the raised errors in the public-facing function are specified in a paragraph.
    image
    As we rely on google-style docstrings, there is no convention to allow lists in the description of the different sections of a docstring. Portions of our API-doc have run-on sentences because someone used a list in the section description which appear as badly formatted sentences publicly.
Intended in API-doc
image image

mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/tests/test_layerwise_folding.py Outdated Show resolved Hide resolved
@purva-thakre
Copy link
Collaborator Author

purva-thakre commented Jun 28, 2024

This PR is ready to be merged once all checks pass.

As of 1:45 pm CST, not all checks are being run after I pushed the latest commit to this PR.

image

Looks like Github is aware of the issues and they are working on fixing it. https://www.githubstatus.com/

image

@cosenal cosenal self-requested a review July 1, 2024 09:41
Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

LGTM

To re-run all checks, you will have to push another commit (even an empty one). I re-run it manually here and everything looks fine.

mitiq/lre/tests/test_layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
Copy link
Contributor

@cosenal cosenal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vprusso vprusso left a comment

Choose a reason for hiding this comment

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

(Minor comments only--but aside from those, LGTM!)

mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
mitiq/lre/multivariate_scaling/layerwise_folding.py Outdated Show resolved Hide resolved
@purva-thakre purva-thakre merged commit acf130c into main Jul 1, 2024
18 checks passed
@purva-thakre purva-thakre deleted the lre_noise_scaling branch July 1, 2024 17:50
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.

LRE Functions: Noise Scaling
4 participants