-
Notifications
You must be signed in to change notification settings - Fork 17
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
Cosmetic/pretty printing of compiler output #599
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
def _replace_placeholders(pretty_output: str) -> str: | ||
"""Replace placeholders for object `repr`s, since black can't parse them.""" | ||
if not self.has_multiple_circuits(): | ||
pretty_output = pretty_output.replace("quantum_circuit", repr(self.circuit)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible/make sense to use the circuit diagrams instead of their reprs, assuming they're small enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (Also moved pretty printing to separate files per chat with Rich.)
I bet edit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm (with micro nit)
Co-authored-by: Emilio Peláez <63567458+epelaaez@users.noreply.github.com>
|
" {cirq.LineQubit(0): cirq.LineQubit(0)},\n" | ||
" ],\n" | ||
" [\n" | ||
" Schedule(\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to abbreviate things that are this long - i think one of the goals of adding a pretty repr is to make it so e.g. typing compiler_output
in a console/notebook just prints something manageable and (somewhat) succinct
(i think textwrap.shorten
does something like this?)
@@ -146,6 +146,23 @@ def __repr__(self) -> str: | |||
f"{self.pulse_lists!r})" | |||
) | |||
|
|||
def __repr_pretty__(self) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be picked up by ipython/jupyter/etc i think this needs to be
def __repr_pretty__(self) -> str: | |
def _repr_pretty_(self, p, cycle=False) -> None: | |
... | |
p.text(<string>) |
(see https://ipython.readthedocs.io/en/stable/config/integrating.html#pretty-printing)
"CompilerOutput(\n" | ||
" [\n" | ||
f" {circuits[0]!r},\n" | ||
" \n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible/easy to avoid these spaces on black *blank* lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean replace them with \t
tab characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops sorry that was supposed to say "blank lines" - my thinking was just that there's no reason to include spaces in otherwise empty lines, e.g. this line could just be
" \n" | |
"\n" |
Done. |
Fixes #144. (edit: Now with circuit drawings.) E.g.,