Skip to content

Support common notations in circuit_to_latex_using_qcircuit - PR for Issue #4685 #7151

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

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

Conversation

NikolaiLong
Copy link

@NikolaiLong NikolaiLong commented Mar 18, 2025

PR for: #4685

Copy link

codecov bot commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.12%. Comparing base (5c198ce) to head (3898f9e).
Report is 173 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7151      +/-   ##
==========================================
+ Coverage   97.88%   98.12%   +0.24%     
==========================================
  Files        1085     1093       +8     
  Lines       95184    95596     +412     
==========================================
+ Hits        93174    93808     +634     
+ Misses       2010     1788     -222     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NikolaiLong
Copy link
Author

I apologize for how long it's taking to resolve formatting issues.

@mhucka
Copy link
Contributor

mhucka commented Mar 19, 2025

I apologize for how long it's taking to resolve formatting issues.

No need for apologies at all!

What would make it easier to catch format issues these earlier? I know from experience that waiting for CI checks to tell me about niggling issues becomes frustrating pretty quickly. Are there ways of making the contribution process easier on people?

@NikolaiLong
Copy link
Author

I see, you're right, running a tool before pushing a commit would speed up this process a lot.

I found the /benchmarks and /check directories in Cirq, and I'm assuming the ./check/all script runs all the checks for formatting.
I tried running the script in my virtual environment - and out of it - after setting the script's permissions to execute (chmod +x ./check/all); however, I can't get the script to run. Whenever I attempt (./check/all), my system tries to simply open the file rather than run it.

Is there any step that I'm missing? I really appreciate the response @mhucka.

@mhucka
Copy link
Contributor

mhucka commented Mar 19, 2025

@NikolaiLong what is the operating system you're using?

@NikolaiLong
Copy link
Author

@mhucka I'm on Windows using PowerShell (Administrator).

@mhucka
Copy link
Contributor

mhucka commented Mar 19, 2025

Ah, hmm, that's probably why they are not working as expected. They were written with the assumption of Linux/MacOS/Unix systems (c.f. https://quantumai.google/cirq/dev/development).

The only thing that comes to mind right now is to try the Windows Subsystem for Linux (WSL). I haven't used it myself, but reading about it, it's supposed to provide a Bash shell, so maybe those scripts will run there.

Would the WSL be an option? I don't know what versions of Windows support it or what other impliications it has.

@NikolaiLong
Copy link
Author

Yes! I have WSL installed on my machine already. I'll use it for the next issue I work on, that'll probably fix this problem. Thanks for helping get to the bottom of this @mhucka!

@verult verult requested a review from mhucka April 2, 2025 17:25
@NikolaiLong NikolaiLong changed the title PR for Issue #4685 Support common notations in circuit_to_latex_using_qcircuit - PR for Issue #4685 Apr 2, 2025
@mhucka mhucka self-assigned this Apr 3, 2025
@mhucka
Copy link
Contributor

mhucka commented Apr 3, 2025

@NikolaiLong very sorry for the delay reviewing this. I seem to be unable to update this branch due to some permissions issue; I don't think it's anything you did or can fix, but I need to figure out what's going on. Planning on working on that and the review today or tomorrow. Update 2025-04-07: never mind; this button was something shown by the extension Refined Github I installed in my browser not long ago. I've turned off this feature so that it doesn't confuse me in the future.

image

@pavoljuhas pavoljuhas added the priority/before-1.5 Finish before the Cirq 1.5 release label Apr 4, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for working on this!

It's really great to provide an example file and rendered output. Would you be open to adding a couple more test cases? The issue #4685 only shows a particular example (namely, the particular case the OP was concerned about), but IMHO the issue is a little broader. This PR is an opportunity to cover a support slightly more operations. Especially welcome would be:

  • gates H, X, Y, Z gates
  • measurement operator

Examples of these gates can be seen in the Qcircuit tutorial and in the Cirq docs about basic circuits & gates. The gates in particular have some similarities, which suggests that the implementation in this PR could be done by looking for commonalities and writing generalized code, rather than very specific code for each individual case.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I can add more test cases and cover these additional gates. I'll get working on that soon!

Comment on lines +199 to +232
# def test_pdf_custom_gates():
# # test for proper rendering of failing latex formats in a pdf
# q0, q1, q2 = cirq.LineQubit.range(3)

# # custom gate with a control for zero and one
# custom_gate = cirq.X.controlled(2, control_values=(0, 1))

# circuit = cirq.Circuit(
# custom_gate(q0, q1, q2),
# custom_gate(q2, q0, q1),
# custom_gate(q1, q2, q0),
# cirq.SWAP(q0, q1),
# cirq.SWAP(q1, q2),
# )

# pdf_kwargs = {
# 'compiler': 'latexmk',
# 'compiler_args': ['-pdf', '-f', '-pdflatex=xelatex', '-quiet'],
# }

# remove = ['aux', 'fdb_latexmk', 'fls', 'log']

# # This test was facing errors due to technical issues in qcuircuit_pdf.py
# # that are out of scope of my issue (4685)
# # It will remain commented out until a resolution is found,
# # or until it is realized that this test is not necessary

# pdf.circuit_to_pdf_using_qcircuit_via_tex(
# circuit,
# "./cirq-core/cirq/contrib/qcircuit/pdf_test_files/test",
# pdf_kwargs,
# None,
# remove
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this commented-out code is in the file. We don't typically want to leave commented-out code in the final versions of things, so it would be best to remove this.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will remove this - I was trying to test PDF output, but the required packages aren't included in the virtual environment set up, so the tests would not be able to run on everyone's machine.

@pavoljuhas
Copy link
Collaborator

I think this needs a bit more time to converge. As a contrib function it is also less critical to appear in next stable release which we want to complete ASAP.
Let us revisit this after 1.5.0.

@pavoljuhas pavoljuhas added priority/after-1.5 Leave for after the Cirq 1.5 release and removed priority/before-1.5 Finish before the Cirq 1.5 release labels Apr 9, 2025
@github-actions github-actions bot added the size: M 50< lines changed <250 label Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/interop area/visualization priority/after-1.5 Leave for after the Cirq 1.5 release size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants