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

Update code of local folding functions #649

Merged
merged 28 commits into from May 2, 2021

Conversation

andreamari
Copy link
Member

@andreamari andreamari commented Apr 21, 2021

Description

Draft PR to:

Checklist

Check off the following once complete (or if not applicable) after opening the PR. The PR will be reviewed once this checklist is complete and all tests are passing.

If some items remain, you can mark this a draft pull request.

Tips

  • If the validation check fails:

    1. Run make check-style (from the root directory
      of the repository) and fix any flake8 errors.

    2. Run make format to format your code with the black
      autoformatter.

    For more information, check the style guidelines for Mitiq.

  • Write "Fixes #XYZ" in the description if this PR fixes Issue #XYZ.

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #649 (81a27cb) into master (fe58997) will increase coverage by 0.07%.
The diff coverage is 98.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   97.30%   97.38%   +0.07%     
==========================================
  Files          35       39       +4     
  Lines        1821     1986     +165     
==========================================
+ Hits         1772     1934     +162     
- Misses         49       52       +3     
Impacted Files Coverage Δ
mitiq/zne/scaling/folding.py 98.91% <98.79%> (+1.60%) ⬆️
mitiq/cdr/test_clifford_training_data.py 100.00% <0.00%> (ø)
mitiq/cdr/__init__.py 100.00% <0.00%> (ø)
mitiq/cdr/clifford_training_data.py 92.66% <0.00%> (ø)
mitiq/mitiq_cirq/cirq_utils.py 100.00% <0.00%> (ø)
mitiq/mitiq_qiskit/conversions.py 96.90% <0.00%> (+0.09%) ⬆️
mitiq/pec/pec.py 97.22% <0.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe58997...81a27cb. Read the comment docs.

@rmlarose rmlarose mentioned this pull request Apr 21, 2021
4 tasks
@andreamari andreamari changed the title [WIP] Update code of local folding functions Update code of local folding functions Apr 26, 2021
@andreamari
Copy link
Member Author

@rmlarose , this is now ready for review.

Main change 1: re-written local folding functions with via folding masks. This also fixes #650.
Main change 2: removed many helper functions which now are not used anymore.

Minor note 1: I made squash_moments private such that now all public functions in zne.folding are valid scaling methods which can be safely imported and used for ZNE. If you don't like this, I can revert it to public. (Maybe utils.py file in mitiq/zne/scaling could be useful in the future).

Minor note 2: you don't need to review fold_global. Even if it is marked as green in the diff, it was actually unchanged.

@andreamari andreamari assigned rmlarose and unassigned rmlarose Apr 27, 2021
@andreamari andreamari marked this pull request as ready for review April 27, 2021 15:09
@@ -124,7 +116,7 @@ def _check_foldable(circuit: Circuit) -> None:
)


def squash_moments(circuit: Circuit) -> Circuit:
def _squash_moments(circuit: Circuit) -> Circuit:
Copy link
Contributor

Choose a reason for hiding this comment

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

No change needed, but we really need to make submodule structure consistent. Here, squash_moments is never imported in zne.scaling.__init__.py, so it is private and the underscore is extraneous. In other places though (zne/inference.py) the underscore is needed because the module structure is different.

Comment on lines 482 to 484
# TODO: decide if removing next else case to get better approximations
else:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

Any updates here @andreamari? What will it take to decide this?

Copy link
Member Author

@andreamari andreamari May 2, 2021

Choose a reason for hiding this comment

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

I removed the TODO (bc75b9f). It was just an idea to improve the scale factor approximation but I think it is better to keep the behavior of the function as simple and predictable as possible to avoid confusing the user.

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.

This is very nice, thanks @andreamari. I'm happy to see 300 fewer lines of code here. See inline comments before merging.

Can be merged without remaining comments, but food for thought and perhaps future work:

  • Should the masks be two-dimensional instead of one-dimensional to match circuit structure? It's easy enough to np.reshape between these, 2d seems a little more natural to me.
  • Should _apply_fold_mask be user-facing? (Involves above comment.) This is quite general and perhaps useful.
  • Should we recommend that custom folding methods be written using _apply_fold_mask?
  • I'm not the biggest fan of folding_method being a string in _create_fold_mask, but I guess it's ok.

@andreamari
Copy link
Member Author

andreamari commented May 2, 2021

About your general comments:

  • Should the masks be two-dimensional instead of one-dimensional to match circuit structure? It's easy enough to np.reshape between these, 2d seems a little more natural to me.

I see your point but I am not sure about it, I think we can discuss about a 2D mask in the future. My fear is that it could make
things more complicated. I see that Cirq use moments (which is a nice thing to handle idle pauses), but it is also true that in theoretical descriptions circuits are naturally written as ordered sequences of operations. Also, for users working with other circuit types (qiskit, pyquil) the 1D mask is perhaps more natural. But happy to discuss and change my mind on this.

  • Should _apply_fold_mask be user-facing? (Involves above comment.) This is quite general and perhaps useful.

I think it is possible. However, before doing this, it would be good to clearly separate "standard" scaling functions (where scale_factor is an argument) from other anomalous functions like this one. Maybe using different files.

  • Should we recommend that custom folding methods be written using _apply_fold_mask?

It can be a suggestion but not a requirement. Sometimes it is not appropriate e.g. fold_global and sometimes a direct approach is probably simpler (e.g. fold_all) .

  • I'm not the biggest fan of folding_method being a string in _create_fold_mask, but I guess it's ok.

As long as it is private I think it is probably fine. If we need something more flexible in the future, we could use a list of indices instead of a string to identify a generic folding order.

@andreamari andreamari requested a review from rmlarose May 2, 2021 15:16
@rmlarose rmlarose merged commit 6c5b394 into master May 2, 2021
@rmlarose rmlarose deleted the more-uniform-scaling-functions branch May 2, 2021 16:58
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.

Make folding at_random and from_right and from_left consistent with odd scale factors
2 participants