Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LRE Executors #2499
LRE Executors #2499
Changes from 10 commits
6614641
3db2817
759fbc5
2c35e74
d0f0052
7708f50
003d3a1
c970474
9146116
d926c5c
595360a
41d08f5
ce46c58
4561acb
3d25cd2
21aeb4d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Let's follow what happens in ZNE here for consistency.
mitiq/mitiq/zne/zne.py
Line 70 in 6f912f6
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 don't use the
num_to_average
param in LRE.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, I'm missing how
num_to_average
is related here. I'm just suggesting we typehint the folding method differently.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.
That's weird. I was viewing this suggestion locally in VS code. I interpreted 'follow what happens in ZNE for consistency' as use the typehint as well as the additional param.
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.
Have you modified code above these lines? If so then it will probably change the location of the suggestion.
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 hadn't modified anything in ZNE which is why it's weird that when you linked L70 in
zne.py
, L71 also showed up.The location of the suggestion was not changed.
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.
Something more in the spirit of unit tests (and more future-proof) would be a test that only checks whether the correct functions are called, and doesn't actually run the circuit through the simulator. Here you are testing lots of things at the same time (including a Google's simulator), all of which could go wrong without giving much insights on the unit you are testing here, which is the behaviour of the new decorator.
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.
@cosenal @natestemen Are either of you available for a quick call on Friday (excluding the community call)? I have been unsuccessful in trying to write a unit test that utilizes mock objects.
It's unclear to me what needs to be a mock object and what must have a pre-defined value.
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 misled you by not creating a variable for
test_cirq * 200
in y'day live coding*, but we do want one in the pull request.*As @natestemen was bugging me about while I tried to hack things around as quick as possible 😅
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 am going to use the mock testing module for this. As Nate noticed yesterday,
test_cirq * 200
will increase the time taken by unit tests.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.
Discussed with Nate and Alessandro to skip this unit for when we get to #2512 in the future.
The code block that misbehaves when we have really large circuits is in one of the inference functions.
mitiq/mitiq/lre/inference/multivariate_richardson.py
Lines 165 to 182 in 6f912f6