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

Update and refresh cirq-* module README files #6900

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mhucka
Copy link
Contributor

@mhucka mhucka commented Dec 31, 2024

This updates all the cirq-* module files to fix some broken image links, spiff up the layout and formatting, and add some additional links here and there.

As before, these remain purposefully short and fairly minimal because these are add-only modules to the main cirq distribution. The main package README will be much fuller.

Due to differences in reStructuredText support between GitHub and PyPI, some desirable attractive formatting cannot be achieved without using raw HTML, which PyPI does not support. I decided to focus on PyPI rather than GitHub for these README files; the rationale is that these modules are separate packages on PyPI and thus have their own landing pages there, which makes them more visible than on GitHub where they are buried in subdirectories of the overall Cirq repo. Consequently, these files look slightly better on PyPI than on GitHub.

Here are a couple of examples of what the result looks like on PyPI:

image image

This updates all the cirq-* module files to fix some broken image
links, spiff up the layout and formatting, and add some additional
links here and there.
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Dec 31, 2024
rstcheck complains about it.
Copy link

codecov bot commented Dec 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.87%. Comparing base (0361a81) to head (c768764).
Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6900   +/-   ##
=======================================
  Coverage   97.87%   97.87%           
=======================================
  Files        1084     1084           
  Lines       94420    94495   +75     
=======================================
+ Hits        92409    92484   +75     
  Misses       2011     2011           

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

Access to Google Hardware is currently restricted to those in an approved group. In order to do this, you will need to apply for access, typically in partnership with a Google sponsor.
This Python module is |cirq-google|, which provides everything you'll need to
run |cirq|_ quantum algorithms on the Google `Quantum Computing Service
<https://quantumai.google/cirq/google/concepts>`__. It also contains
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we clarify here (and maybe in the other vendor packages) that this is only open to authorized partners? (i.e. it's not everything you need)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of the revised version?

There was a boxed paragraph two paragraphs below the paragraph where the text above appears; I moved that box right under the first paragraph so that the note is more obvious. Please let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good to me. What do you think of adjusting text for the other vendor modules as well? We could do a much more simpler change, such as changing "this module contains everything you need for ..." to "this module contains all the code you need for... See for more information on getting a license key for this service"

@mhucka mhucka enabled auto-merge (squash) January 14, 2025 20:13
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of small suggestions - please see inline.
Thank you for improving these!

Comment on lines +50 to +53
* To install the latest pre-release version of |cirq-aqt|, use ``pip install
cirq-aqt~=1.0.dev``. (The ``~=`` has a special meaning to ``pip`` of
selecting the latest version compatible with the ``1.*`` and ``dev`` in the
name. Despite appearances, this will not install an old version 1.0 release!)
Copy link
Collaborator

@pavoljuhas pavoljuhas Jan 16, 2025

Choose a reason for hiding this comment

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

Minor point - let us have installation command on one line so it is easier to copy/paste from GitHub-rendered README. The current text pastes as an invalid two-line shell command.
Also please check for similar line-wrapped commands in other README-s.

(Format only, no change in the text)

Suggested change
* To install the latest pre-release version of |cirq-aqt|, use ``pip install
cirq-aqt~=1.0.dev``. (The ``~=`` has a special meaning to ``pip`` of
selecting the latest version compatible with the ``1.*`` and ``dev`` in the
name. Despite appearances, this will not install an old version 1.0 release!)
* To install the latest pre-release version of |cirq-aqt|, use
``pip install cirq-aqt~=1.0.dev``
(The ``~=`` has a special meaning to ``pip`` of selecting the latest version
compatible with the ``1.*`` and ``dev`` in the name. Despite appearances,
this will not install an old version 1.0 release!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea. (I think I only did it that way because the originals were as well. But it's better the way you suggest.

Comment on lines +20 to +21
.. |cirq-aqt| replace:: ``cirq-aqt``
.. |cirq-core| replace:: ``cirq-core``
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - do we need these substitutions if they are nearly the same as their expansion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is easier to achieve consistent formatting throughout a document when substitutions are defined. My experience is that when foo is used all over, sooner or later it's forgotten in some places, and the document starts to look sloppy. Also, it makes it easier to change or add formatting (such as hyperlinks). It's the same principle as defining macros for common terms in LaTeX. (I'm sure this is all familiar and I'm just overexplaining …)

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 said, I'm not strongly bothered by getting rid of the substitutions and just doing foo. These are short documents, after all.

.. |cirq-aqt| replace:: ``cirq-aqt``
.. |cirq-core| replace:: ``cirq-core``

.. class:: centered
Copy link
Collaborator

Choose a reason for hiding this comment

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

With apologies for bike-shedding - could we perhaps just leave the left-aligned default?

It would make the text look the same on GitHub and PyPi and would be perhaps more consistent with the logo-on-the-left layout at https://quantumai.google/cirq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With respect, I don't think there's any need to match that page.

Installation
------------

|cirq-web| is currently in development, and therefore is only available in
Copy link
Collaborator

Choose a reason for hiding this comment

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

cirq-web is actually available as a "stable" package at https://pypi.org/project/cirq-web/ and it installs together with the cirq metapackage. The installation info should be similar as for the other sub-packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, woops, that text needs to be corrected!

Comment on lines +97 to +98


Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe remove these blanks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using 2 blank lines before section headings, just as a convention. It's not uncommon if you all prefer single blank lines, that's okay with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants