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

docs format changes #2139

Merged
merged 15 commits into from
Nov 1, 2024
Merged

docs format changes #2139

merged 15 commits into from
Nov 1, 2024

Conversation

assafb
Copy link
Contributor

@assafb assafb commented Oct 15, 2024

No description provided.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@qiskit-bot
Copy link
Contributor

Thanks for contributing to Qiskit documentation!

Before your PR can be merged, it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. Thanks! 🙌

One or more of the following people are relevant to this code:

docs/guides/qedma-qesem.ipynb Outdated Show resolved Hide resolved
docs/guides/qedma-qesem.ipynb Outdated Show resolved Hide resolved
docs/guides/qedma-qesem.ipynb Outdated Show resolved Hide resolved
docs/guides/qedma-qesem.ipynb Outdated Show resolved Hide resolved
"\n",
"| Name | Type | Description | Required | Default | Example |\n",
"| -----| ------| ------------| -------- | ------- | -------- |\n",
"| `pubs` | [EstimatorPubLike](https://github.com/Qiskit/qiskit/blob/5121a0fda8d4e49d47d16a28e1d21046a09d008b/qiskit/primitives/containers/estimator_pub.py#L202-L207) |This is the main input. The `Pub` contains 2-4 elements: a circuit, one or more observables, 0 or a single set of parameter values, and an optional precision. If a precision was not specified, then the `default_precision` from `options` will be used| Yes| N/A | `[(circuit, [obs1,obs2,obs3], parameter_values, 0.03)]` |\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The link to this should be /guides/primitive-input-output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"\n",
"| Option | Choices | Description | Default |\n",
"| -----| -----------| -------- | ------- | \n",
"| `estimate_time_only` | True / False | Specifies whether to execute the mitigation process or only get QPU time estimation | True|\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean estimate_time_only is True by default, and does not run on hardware by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pandasa123 That is correct. The reason is that error mitigation inherently comes with an exponential overhead, so it's better for the user to request execution explicitly to avoid mistakes where they might start the process without realizing it could consume several hours of their monthly QPU time quota. Moreover, in our SaaS offering, we have implemented a two-stage process where the user cannot begin execution without first estimating the QPU usage. Consider it as a safety switch which protects the user from making mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@assafb, just checked with the team. estimate_time_only should be False by default to be consistent across all CFs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"| `0` | Minimal transpilation: the mitigated circuit will closely resemble the input circuit structurally. Circuits provided in level 0 should match the device connectivity and should be specified in terms of the following gates: CX, Rzz(α), and standard single-qubit gates (U, x, sx, rz, etc). Barriers will be respected in the layerification step. |\n",
"\n",
"<Admonition type=\"note\">\n",
"Qiskit barriers are typically used to specify the layers of two-qubit gates in quantum circuits. In level 0, QESEM preserves the layers specified by the barriers. In level 1, the layers specified by the barriers are considered as one transpilation alternative when minimizing QPU time.\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "Qiskit barriers" feels odd? This should read "In Qiskit, barriers"?

CC: @abbycross

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know how you recommend to phrase it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about "Barrier operations are typically used..." (just leave off Qiskit altogether)?

" }\n",
")"
" metadata={}\n",
")\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be helpful to have code showing how to use the primitive results, instead of showing only the type. E.g. print(f"Expectation value from pub 0 is {result[0].data.evs}")

@abbycross
Copy link
Collaborator

I tried to make a change directly to your branch but because it's a fork I may not have the right permissions. Instead, I opened a PR with my own fork to push a change to your fork :) Qedma#1

The needed change is to add some terms to the spelling dictionary

docs/guides/qedma-qesem.ipynb Show resolved Hide resolved
@@ -37,40 +37,56 @@
"id": "74823696",
Copy link
Collaborator

@blakejohnson blakejohnson Oct 18, 2024

Choose a reason for hiding this comment

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

default_precision needs to define a default as it says what to do if a precision is not specified by a PUB.


Reply via ReviewNB

@@ -37,40 +37,56 @@
"id": "74823696",
Copy link
Collaborator

@blakejohnson blakejohnson Oct 18, 2024

Choose a reason for hiding this comment

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

Having an example where stds includes values > 1 reads strangely. Was that intentional?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a typo. Fixed

@frankharkins
Copy link
Member

frankharkins commented Oct 29, 2024

For some reason I can't push to this branch. To fix the lint error, someone will need to run the following commands:

git pull https://github.com/Qiskit/documentation.git main
# You should now be in a merge conflict. To fix, reset the notebook and re-lint it.
git restore docs/guides/qedma-qesem.ipynb --source 50ed9daeb09712c8eb0db384eb3946ed84f1a7f1
tox -e fix
git add .
git commit

EDIT: Alternatively, you can also merge Qedma#2

@pacomf
Copy link
Member

pacomf commented Oct 31, 2024

@assafb can you address previous comment about lint issues to merge the PR? :)

@assafb
Copy link
Contributor Author

assafb commented Nov 1, 2024

@pacomf @frankharkins I merged Qedma#2

@frankharkins frankharkins added this pull request to the merge queue Nov 1, 2024
Merged via the queue into Qiskit:main with commit ff0bd17 Nov 1, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants