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

Allow for Executor class and batched Executors for LRE #2676

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bdg221
Copy link
Collaborator

@bdg221 bdg221 commented Feb 20, 2025

Fixes #2668
Fixes #2675

This PR updates the LRE technique to allow for Executor class objects to be passed in to execute_with_lre. Along with that update, there is also an update to allow for a batched executor.

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.

  • I added unit tests for new code.
  • I used type hints in function signatures.
  • I used Google-style docstrings for functions.

@bdg221 bdg221 self-assigned this Feb 20, 2025
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.74%. Comparing base (f404e28) to head (74b8641).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2676   +/-   ##
=======================================
  Coverage   98.74%   98.74%           
=======================================
  Files          93       93           
  Lines        4219     4223    +4     
=======================================
+ Hits         4166     4170    +4     
  Misses         53       53           

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

Copy link
Member

@natestemen natestemen left a comment

Choose a reason for hiding this comment

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

Great work!

A suggestion for the tests: we should steer away from testing accuracy in the tests. This type of test often leads to non-deterministic behavior which causes spurious failures. I'd recommend two different tests instead:

  1. Ensure the evaluate method has been called $n$ times on the Executor object when the executor cannot batch, and there are $n$ circuits to run.
  2. Ensure the evaluate method is only called once when the executor can batch.

WDYT?

num_chunks,
)
executor_obj = Executor(executor)
if not executor_obj.can_batch:
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the pattern we have in zne/zne.py, but I think it's more readable to have

if executor_obj.can_batch:
    ...
else:
    # cannot batch

@purva-thakre purva-thakre removed their request for review February 21, 2025 16:31
@bdg221
Copy link
Collaborator Author

bdg221 commented Feb 21, 2025

@natestemen I like your idea for the tests. Unfortunately, for testing mitigate_executor, we cannot check calls_to_executor because it does not have an accessible Executor object. The tests for that scenario use accuracy (ZNE, PEC, DDD). If we wanted to avoid accuracy for the mitigate_executor, the code would need to change across the techniques.

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.

Adding batched Executor functionality to LRE Support Executor class in LRE
2 participants