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

Parameter sweeps polish #5515

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

augustehirth
Copy link
Collaborator

Some phrasing changes. Ran a formatter. Cleared outputs in Simulation

@augustehirth augustehirth requested review from a team, vtomole and cduck as code owners June 14, 2022 11:46
@augustehirth augustehirth requested a review from viathor June 14, 2022 11:46
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Jun 14, 2022
@augustehirth augustehirth requested a review from dabacon June 14, 2022 11:47
@@ -102,10 +102,10 @@
"source": [
"q0 = cirq.LineQubit(0)\n",
"\n",
"circuit1 = cirq.Circuit([cirq.H(q0), cirq.Z(q0)**0.5, cirq.H(q0), cirq.measure(q0)])\n",
"circuit1 = cirq.Circuit([cirq.H(q0), cirq.Z(q0) ** 0.5, cirq.H(q0), cirq.measure(q0)])\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Which formatter did you run? We format the main cirq code with black 22.1 which does not include spaces around ** (see #5157).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I used black and black[jupyter]. Running black --version results in black, 22.3.0 (compiled: yes). Should I downgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't need to downgrade because that's the version we use currently; I was mistaken about it being 22.1 (see https://github.com/quantumlib/Cirq/blob/master/dev_tools/requirements/deps/format.txt). I'm a bit surprised that black[jupyter] seems to be treating notepad code differently, but I don't have too much experience with that. It would be nice to integrate notebook formatting into the check/format-incremental script and our CI checks so that notebook code formatting is handled automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK to leave it as-is for now then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I guess I'd prefer to revert the padding around ** to be consistent with cirq code, but up to you.

docs/params.ipynb Outdated Show resolved Hide resolved
Co-authored-by: Matthew Neeley <[email protected]>
Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

Minor comments, then LGTM. Up to you if you'd like to revert the change to whitespace around **.

docs/params.ipynb Outdated Show resolved Hide resolved
docs/simulation.ipynb Outdated Show resolved Hide resolved
docs/simulation.ipynb Outdated Show resolved Hide resolved
docs/simulation.ipynb Outdated Show resolved Hide resolved
@augustehirth augustehirth merged commit 88f3030 into quantumlib:master Jun 14, 2022
@augustehirth augustehirth deleted the parameter_sweeps_polish branch June 14, 2022 20:10
augustehirth added a commit to augustehirth/Cirq that referenced this pull request Jun 15, 2022
Phrasing changes, ran black formatter, cleared outputs in Simulation
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Phrasing changes, ran black formatter, cleared outputs in Simulation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants