-
-
Notifications
You must be signed in to change notification settings - Fork 166
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 workflow images in documentation #2034
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2034 +/- ##
==========================================
- Coverage 98.21% 98.19% -0.02%
==========================================
Files 87 88 +1
Lines 4135 4166 +31
==========================================
+ Hits 4061 4091 +30
- Misses 74 75 +1 ☔ View full report in Codecov by Sentry. |
If anyone uses a browser other than Chrome or Firefox, it's possible the SVG images don't appear as intended. |
c7743e2
to
27334b0
Compare
This is ready for a review. |
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.
Nice work @purva-thakre, this is a great addition and future timesaver!
Minor comment on the general workflow diagram, it looks a little inconsistent between "Step 1" and "2. Inference". It might help to put "Step 1" in brackets or parentheses, or some other way of indicating that it is a placeholder, unlike "2. Inference" which is consistent across techniques. Maybe even just making the font and size/shape of the boxes the same would be enough to make the diagram look unified.
a468803
to
912329e
Compare
@Misty-W I have tried to make the font type consistent. I will ping you when this is ready for a review later. I still need to make the size and shape of the boxes consistent because the text style is still a bit 'off'. |
hi @purva-thakre, I wanted to give a heads up that the Mitiq milestone is closing this Thurs (before I go on vacation 😄). I think the only things remaining to address before merging are Nate's comment about the shadows workflow and my suggestion of how to make the |
@Misty-W Let's move this to the next milestone. I have been working on other issues and I do want to spend some additional days on this. In addition to Nate's comment, I also want to work on understanding how text scales in Inkscape. The text is still a bit 'off'. Thank you for your comment on how to make the download link work! Enjoy your vacation! |
Sounds good thanks @purva-thakre! |
Co-authored-by: Misty Wahl <[email protected]>
b84c740
to
870420e
Compare
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.
New images are looking really nice! I love the aesthetic.
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 think we "insert [a] noise channel" in the first dark green box, do we?
- I think it should read
mitiq.shadows
in the center box instead ofmitiq.classicalshadows
.
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 think we "insert [a] noise channel" in the first dark green box, do we?
Yes, we do. According to the docs. See step 1 in this section of the docs.
I think it should read mitiq.shadows in the center box instead of mitiq.classicalshadows
Thanks for catching that! I did not double-check the API-doc. Only followed what the previous images stated.
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.
Yes, we do. According to the docs. See step 1 in this section of the docs.
That is the "user defined input" section, however. I think that part of the code to ensure we are using a realistic executor. That is not something that happens in the mitiq.shadows
module, which this diagram is explaining.
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.
Would you prefer to have Insert noisy channel
moved to the hardware region?
Otherwise, I don't know how to explain the difference between the workflows for ideal classical shadows and robust shadows. The way I understand it, we use the latter in the former after the device noise is characterized.
OR Step 2
could be changed to Noisy Z-basis measurement
where the noise is user-defined or the one characterized from the robust shadows workflow.
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 see this diagram as explaining what happens under the hood, and I cannot find anywhere in the mitiq.shadows
module where we add a noise channel. I can see we apply random Paulis in shadow_quantum_processing
, but don't see any insertion/application of a noise channel, which is why I think that part may be unnecessary. If you agree this is the purpose of the image, can you show me where I'm missing this part?
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 personally can't understand the details of robust shadow
in Mitiq. I looked at what the main reference of this technique does. https://arxiv.org/abs/2011.09636
Step 7 is where the device noise is characterized.
Steps 8, 9 and 10 are where the classical shadows workflow comes into play.
How about I remove the insert noisy channel
part and make changes to Step 2
? It would then be labeled as Noisy Z-basis measurement
.
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.
Just in case anyone is following along, Purva and I chatted and agreed her suggestion sounds good. We will go forward with that.
I am going ahead and merging this. There is a drop in coverage but the uncovered lines are not related to this PR. |
Fixes #2025
To Do:
mitiq.classicalshadows
.error-mitigated
fromerror mitigated expectation value
.