qiboteam / qibo

A framework for quantum computing
https://qibo.science
Apache License 2.0
287 stars 58 forks source link

Allow `Circuit.draw` to display string directly #1434

Closed renatomello closed 1 week ago

renatomello commented 3 weeks ago

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:

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/qiboteam/qibo/pull/1434/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/qiboteam/qibo/pull/1434/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam) | `97.09% <100.00%> (-0.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=qiboteam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alecandido commented 3 weeks ago

@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.

renatomello commented 1 week ago

@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.

Done. I'd like to push this to be merged before the next release, so then we can deprecate it by release 0.2.13.

alecandido commented 1 week ago

@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 commented 1 week ago

@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.