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

Adds QSE user guide #1976

Merged
merged 6 commits into from
Oct 18, 2023
Merged

Adds QSE user guide #1976

merged 6 commits into from
Oct 18, 2023

Conversation

bubakazouba
Copy link
Contributor

Description

Adds QSE user guide. closes #1924


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.

@codecov
Copy link

codecov bot commented Aug 27, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (68b2b12) 98.30% compared to head (c538a2e) 98.30%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1976   +/-   ##
=======================================
  Coverage   98.30%   98.30%           
=======================================
  Files          87       87           
  Lines        4118     4118           
=======================================
  Hits         4048     4048           
  Misses         70       70           

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

@purva-thakre
Copy link
Collaborator

To fix the docs build error, I think you might have to re-label the spot where you use the QSE workflow diagram for the second time to a different name.

https://github.com/unitaryfund/mitiq/actions/runs/5988506263/job/16243757058?pr=1976#step:6:281

I don't know if there's an easier way around this issue but this is how I quickly fixed it in the past.

@bubakazouba
Copy link
Contributor Author

To fix the docs build error, I think you might have to re-label the spot where you use the QSE workflow diagram for the second time to a different name.

https://github.com/unitaryfund/mitiq/actions/runs/5988506263/job/16243757058?pr=1976#step:6:281

I don't know if there's an easier way around this issue but this is how I quickly fixed it in the past.

Thanks I was a bit confused by this error. Just resolved

@bubakazouba
Copy link
Contributor Author

I thought I changed the label but I'm still getting the same error, am I missing something?

@natestemen natestemen self-requested a review September 1, 2023 16:19
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.

Nice work Sahmoud. This is going to be great to share to external (and internal) users!

  1. We do want this code to be run as part of the make docs build command, so make sure the code cells use the correct syntax:
```{code-cell} ipython3
# your code
```

Check out the underlying ZNE guide source.

  1. I think it's best practice to capitalize Hamiltonian.

docs/source/guide/qse-1-intro.md Outdated Show resolved Hide resolved
docs/source/img/qse-data-flow-diagram.png Outdated Show resolved Hide resolved
docs/source/guide/qse-1-intro.md Outdated Show resolved Hide resolved
docs/source/guide/qse-1-intro.md Outdated Show resolved Hide resolved
docs/source/guide/qse-1-intro.md Outdated Show resolved Hide resolved
Returns:
The expectation value estimated with QSE.
```
The inclusion of the cache significantly speeds up the runtime and avoids the need for recomputation of already computed values. Furthermore, it is important to note that the cache gets modified in place. Specifically, the cache be reused between different executions of the program.
Copy link
Member

Choose a reason for hiding this comment

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

Let's use the warning directive to warn users that this dictionary gets modified in place. Something like this:

```{warning}
warning message about in place modification
```

More info about directives can be found here.

Also, can you clarify the last sentence "Specifically, the cache be reused between different executions of the program."?

Copy link
Member

@natestemen natestemen Sep 27, 2023

Choose a reason for hiding this comment

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

Needs addressed. related to #1976 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the warning message.

I'm not sure how to clarify the last sentence. I'm trying to say that the user can use the pass the same cache object to execute_with_qse for example if they are assuming the same noise model between executions.

docs/source/guide/qse-4-low-level.md Outdated Show resolved Hide resolved
docs/source/guide/qse-4-low-level.md Outdated Show resolved Hide resolved
docs/source/guide/qse.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

@andreamari can you help me out a bit with this review and take a look through this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In hindsight this is very obvious, but I just realized now that I should have written all the code in notebooks then exported the .md file from there. I have been editing the files directly in the .md files from the zne examples as templates.

@Misty-W
Copy link
Contributor

Misty-W commented Sep 7, 2023

hi @natestemen and @bubakazouba, checking if this is on track to close by tomorrow. No worries if not, just want to know whether to include it in milestone 0.29, which closes tomorrow.

@nathanshammah nathanshammah modified the milestone: 0.30.0 Sep 13, 2023
Copy link
Member

@andreamari andreamari 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 a review of just the theory section. Looks great!
Minor comments are below.

Optional additional comment:
In qse-1-intro.md the function qse_demo() is defined line by line and finally executed. Do you think removing qse_demo() and directly using its body when necessary is better? This is just to make a conscious decision, I am fine with any choice.

docs/source/guide/qse-5-theory.md Outdated Show resolved Hide resolved
docs/source/guide/qse-5-theory.md Outdated Show resolved Hide resolved
docs/source/guide/qse-5-theory.md Outdated Show resolved Hide resolved
@nathanshammah
Copy link
Member

@natestemen checking with you if this can be approved or there are further changes to ask to @bubakazouba, after @andreamari's review, which provided some suggested edits but approved the theory part.

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.

Few more changes needed before going forward. Very close!

docs/source/guide/qse-1-intro.md Show resolved Hide resolved

## Advantages

The advantage of QSE is that it allows determining the value of excited electronic states without the need for a formal error-correcting procedure. Furthermore, the method is error-agnostic. Computationally, the advantage is eliminating the need for ancilla measurements.
Copy link
Member

@natestemen natestemen Sep 27, 2023

Choose a reason for hiding this comment

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

This needs addressed. This comment is confusingly displayed by itself, but is related to my previous comment #1976 (comment)

Returns:
The expectation value estimated with QSE.
```
The inclusion of the cache significantly speeds up the runtime and avoids the need for recomputation of already computed values. Furthermore, it is important to note that the cache gets modified in place. Specifically, the cache be reused between different executions of the program.
Copy link
Member

@natestemen natestemen Sep 27, 2023

Choose a reason for hiding this comment

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

Needs addressed. related to #1976 (comment)

docs/source/guide/qse-3-options.md Outdated Show resolved Hide resolved
docs/source/guide/qse-4-low-level.md Outdated Show resolved Hide resolved
@Misty-W
Copy link
Contributor

Misty-W commented Oct 13, 2023

hi @bubakazouba, still interested in working on the QSE docs?

No worries if you're too busy right now, we can finish this up for you so everyone can see the great work you've done 💯

@bubakazouba
Copy link
Contributor Author

hi

Hi Misty, sorry for the slow work on this. I will wrap it up this weekend.

@bubakazouba
Copy link
Contributor Author

hi @natestemen and @bubakazouba, checking if this is on track to close by tomorrow. No worries if not, just want to know whether to include it in milestone 0.29, which closes tomorrow.

Sorry for the delay! All comments should be addressed now.

Copy link
Contributor

@Misty-W Misty-W left a comment

Choose a reason for hiding this comment

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

Last wording changes to address Nate's comments.

@Misty-W Misty-W dismissed natestemen’s stale review October 17, 2023 16:53

I've addressed the comments in the wording change suggestions, which I've since applied to this PR.

@Misty-W
Copy link
Contributor

Misty-W commented Oct 17, 2023

Sorry for the delay! All comments should be addressed now.

Thanks for finishing this up @bubakazouba!
Once the docs are built successfully, this PR will be ready to merge.
Btw, even if Read the Docs (RTD) shows a red "x", click on the "details" link to check the real status.
If you can view the docs and they render correctly, we consider that to be passing.
The red "x" shows up for RTD sometimes because our docs take a long time to build. We have a separate issue open to address that.

@Misty-W Misty-W merged commit 0efb0ef into unitaryfund:master Oct 18, 2023
20 checks passed
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.

Write documentation for QSE in the Mitiq user guide
6 participants