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

Allow Circuit.draw to display string directly #1434

Merged
merged 41 commits into from
Sep 19, 2024
Merged

Allow Circuit.draw to display string directly #1434

merged 41 commits into from
Sep 19, 2024

Conversation

renatomello
Copy link
Contributor

@renatomello renatomello commented Sep 4, 2024

This PR proposes enabling the Circuit.draw method to print the circuit without having to wrap it inside a print() call. Example:

In [1]: from qibo import Circuit, gates, set_backend

In [2]: circuit = Circuit(2)

In [3]: circuit.draw()
q0: ─
q1: ─

In [4]: circuit.draw(output_string=True)
Out[4]: 'q0: ─\nq1: ─'

In [3]: print(circuit.draw(output_string=True))
q0: ─
q1: ─

Checklist:

  • Reviewers confirm new code works as expected.
  • Tests are passing.
  • Coverage does not decrease.
  • Documentation is updated.

@renatomello renatomello self-assigned this Sep 4, 2024
@renatomello renatomello added the enhancement New feature or request label Sep 4, 2024
@renatomello renatomello modified the milestones: Qibo 0.2.13, Qibo 0.2.12 Sep 4, 2024
@renatomello renatomello modified the milestones: Qibo 0.2.12, Qibo 0.2.13 Sep 4, 2024
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.09%. Comparing base (1b6eb8a) to head (ce8ca3e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1434      +/-   ##
==========================================
- Coverage   97.11%   97.09%   -0.03%     
==========================================
  Files          81       81              
  Lines       11679    11684       +5     
==========================================
+ Hits        11342    11344       +2     
- Misses        337      340       +3     
Flag Coverage Δ
unittests 97.09% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@renatomello renatomello marked this pull request as ready for review September 5, 2024 07:16
@alecandido
Copy link
Member

@renatomello following Python standards, I'd propose to use Circuit.__str__() to return the stringified representation of the circuit, and we can then convert Circuit.draw() to directly print (avoiding extra parameters in both cases).
Notice that some strings have to be closer to a code representation, but this is not the case for __str__, but rather __repr__'s role

However, notice that this would be a breaking change, as all the assignment to the result of circuit.draw() will receive None.

So, we could also schedule .draw() replacement, but for the time being, I'd suggest using a different name for this feature, something like Circuit.display() or Circuit.print() itself.

tests/conftest.py Outdated Show resolved Hide resolved
src/qibo/models/circuit.py Outdated Show resolved Hide resolved
@alecandido
Copy link
Member

@renatomello a9bca8 is exactly what I meant.

I'm not sure why you're prepending the underscores: I know that .display() is temporary, and it will be removed, but you may want to use it in the meanwhile (and we will need to break anyhow to change the meaning of .draw(), breaking one or two doesn't make much difference).
Instead, the .diagram() call is there to stay, since it will be the only way to directly access the string, even though it won't be used that frequently (ok, the name is random, I was out of ideas and went looking on Thesaurus for inspiration - if you have any better proposal it's perfectly fine).

Other than that, the only bit missing to this PR is to convert the already converted usages, since now print(c.draw()) has been converted to just c.draw(), but it's not (yet) going to work as intended.

@renatomello
Copy link
Contributor Author

@renatomello a9bca8 is exactly what I meant.

I'm not sure why you're prepending the underscores: I know that .display() is temporary, and it will be removed, but you may want to use it in the meanwhile (and we will need to break anyhow to change the meaning of .draw(), breaking one or two doesn't make much difference). Instead, the .diagram() call is there to stay, since it will be the only way to directly access the string, even though it won't be used that frequently (ok, the name is random, I was out of ideas and went looking on Thesaurus for inspiration - if you have any better proposal it's perfectly fine).

Other than that, the only bit missing to this PR is to convert the already converted usages, since now print(c.draw()) has been converted to just c.draw(), but it's not (yet) going to work as intended.

right now print(circuit.draw()) still works as before and circuit.display works in-place, However, circuit.draw raises a warning saying that it will work in-place in the future.

Copy link
Member

@alecandido alecandido left a comment

Choose a reason for hiding this comment

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

right now print(circuit.draw()) still works as before and circuit.display works in-place, However, circuit.draw raises a warning saying that it will work in-place in the future.

Yes, that's correct, I perfectly agree with you.

It's just that some statements print(circuit.draw()) have been replaced by just circuit.draw(). While, since you're now preserving the old behavior, they should be unchanged (or invoke .display() instead)

doc/source/code-examples/advancedexamples.rst Outdated Show resolved Hide resolved
Co-authored-by: Alessandro Candido <[email protected]>
@renatomello renatomello added this pull request to the merge queue Sep 19, 2024
Merged via the queue into master with commit 2d4e077 Sep 19, 2024
25 checks passed
@renatomello renatomello deleted the draw branch September 19, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants