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

Quick fix: Fix typo and headings in classical shadows tutorial #2574

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

JMuff22
Copy link
Contributor

@JMuff22 JMuff22 commented Nov 14, 2024

Description

I noticed some typos and an unrendered heading in the classical shadows documentation. These are fixed here.

Additionally, if you (the maintainers) think it useful I think it would be good to highlight Remark 1 from the paper (https://arxiv.org/abs/2002.08953)

Remark 1 (Constants in Theorem 1). The numerical constants featuring in N and K result from a conservative
(worst case) argument that is designed to be simple, not tight. We expect that the actual constants are much
smaller in practice.

In the shadows_tutorial for equation (10). I think the sudden appearance of 34 is confusing in the guide.

image


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

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello @JMuff22, thank you for submitting a PR to Mitiq! We will respond as soon as possible, and if you have any questions in the meantime, you can ask us on the Unitary Fund Discord.

@JMuff22 JMuff22 changed the title Fix typo and headings in classical shadows tutorial Quick fix: Fix typo and headings in classical shadows tutorial Nov 14, 2024
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.72%. Comparing base (9561b9f) to head (80067a2).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2574   +/-   ##
=======================================
  Coverage   98.72%   98.72%           
=======================================
  Files          92       92           
  Lines        4168     4168           
=======================================
  Hits         4115     4115           
  Misses         53       53           

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


🚨 Try these New Features:

@purva-thakre
Copy link
Collaborator

Hi @JMuff22, thank you for your correction! Cleaning up this tutorial is on our list of todos #2110

Additionally, if you (the maintainers) think it useful I think it would be good to highlight Remark 1 from the paper

I have a very high-level understanding of how this technique works. Is there a particular spot where this remark might be useful?

In the shadows_tutorial for equation (10). I think the sudden appearance of 34 is confusing in the guide.

I think this equation is directly taken from the paper. See eq S13 in the supplementary info.

@JMuff22
Copy link
Contributor Author

JMuff22 commented Nov 16, 2024

I think this equation is directly taken from the paper. See eq S13 in the supplementary info.

Yes S13 without remark 1 is confusing in my opinion.

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.

Good catch! Thanks for the patch.

I agree the remark would be helpful here. Is that something you'd like to add to this PR?

@JMuff22
Copy link
Contributor Author

JMuff22 commented Nov 19, 2024

By the way, is there a quicker way of building the documentation for quick fixes? I noticed that make docs builds and executes all the jupyter notebooks which is not ideal for testing how a small change like this would look when rendered.

@JMuff22
Copy link
Contributor Author

JMuff22 commented Nov 19, 2024

Good catch! Thanks for the patch.

I agree the remark would be helpful here. Is that something you'd like to add to this PR?

Added my suggested fix.

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.

Thanks Jake! BTW are you using the mitiq.shadows module in practice? Would love to get your thoughts/opinions on how it's going if so.

@natestemen
Copy link
Member

By the way, is there a quicker way of building the documentation for quick fixes? I noticed that make docs builds and executes all the jupyter notebooks which is not ideal for testing how a small change like this would look when rendered.

make docs will cache the results from notebooks, so if it doesn't change it won't rerun. There's no caching on CI, but locally it should speed up after the first build. I think @cosenal had a command to run a single file, but I cannot find it now.

@natestemen natestemen merged commit 56e7c00 into unitaryfund:main Nov 19, 2024
15 of 16 checks passed
@JMuff22
Copy link
Contributor Author

JMuff22 commented Nov 20, 2024

Thanks Jake! BTW are you using the mitiq.shadows module in practice? Would love to get your thoughts/opinions on how it's going if so.

I am not really using it in practice. Maybe in the future. I was more using it to learn the protocol and apply it myself.

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.

3 participants