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

Pretty print #245

Open
wants to merge 128 commits into
base: master
Choose a base branch
from
Open

Pretty print #245

wants to merge 128 commits into from

Conversation

benjvmin93
Copy link
Contributor

Before submitting, please check the following:

  • Make sure you have tests for the new code and that test passes (run nox)
  • If applicable, add a line to the [unreleased] part of CHANGELOG.md, following keep-a-changelog.
  • Format added code by ruff
    • See CONTRIBUTING.md for more details
  • Make sure the checks (github actions) pass.

Then, please fill in below:

Context (if applicable):

#239

Description of the change:

New draw() methods in Circuit and Pattern classes.

  • Circuit.draw() generates the corresponding Qasm3 code and pass it to qiskit.draw() method. It relates on the same arguments (see qiskit.draw() method documentation).
  • pattern.draw() locally generates the appropriate visualization object (in the ascii, unicode, png, or latex formats)

More details on the different changes are described in the CHANGELOG.md file.

Related issue:

benjvmin93 and others added 30 commits April 8, 2024 17:15
…ng: see tests_patterns, tests_tnsim and tests_transpiler
@benjvmin93
Copy link
Contributor Author

One last thing that prevents the checks to be successful is the adding of qiskit module for drawing circuit.
The test_ibmq_runner test is not skipped anymore since the qiskit module is present and fails because of the absence of the save_statevector() method, which I suppose, is due to Qiskit changes in the new versions.

I didn't want to change anything regarding this, since my work didn't involve anything in the runner tests so maybe we should discuss how to correct this issue to make sure we agree on what to do. Maybe either by replacing this method with the new appropriate one (in the Aer module I think, so without calling it from a QuantumCircuit object) or even treating this issue in a new PR.

@thierry-martinez

@shinich1
Copy link
Contributor

@benjvmin93 was qiskit added to required module in this PR? I agree updating tests to be compatible with latest qiskit would be good.
@thierry-martinez what do you think is the first thing to do? completing TeamGraphix/graphix-ibmq#16 before updating tests etc in this repo?

@@ -20,6 +20,62 @@
Node = int


def command_to_latex(cmd: Command) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to be implemented as methods?

import PIL

try:
subprocess.run(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's setup ruff rule S to check insecure operations.

"""Return a string representation of the Circuit."""
return self.draw()

def draw(self, output: str = "text") -> TextDrawing | matplotlib.figure | PIL.Image | str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use @typing.overload for better UX.

@@ -6,7 +7,9 @@ networkx>3.0
numpy>=1.24,<2
opt_einsum>=3.2
pydantic
pylatexenc
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this optional since all users need to set up LaTeX if pylatexenc is listed here.

quimb>=1.4.0
scipy
sympy>=1.9
typing_extensions
qiskit_qasm3_import
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above (import only when necessary)

@@ -668,3 +669,22 @@ def test_remove_qubit(self) -> None:

def assert_equal_edge(edge: Sequence[int], ref: Sequence[int]) -> bool:
return any(all(ei == ri for ei, ri in zip(edge, other)) for other in (ref, reversed(ref)))


def test_draw_pattern():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_draw_pattern():
def test_draw_pattern() -> None:



@pytest.mark.skipif(which("latex") is None, reason="latex not installed")
def test_draw_pattern_latex():
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def test_draw_pattern_latex():
def test_draw_pattern_latex() -> None:

@EarlMilktea
Copy link
Contributor

MEMO: It may better to make big packages optional (ex. qiskit, matplotlib, etc.) for better usability.

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.

4 participants