Fix #853: improve formatter for pretty-printing musical score diagrams

[jbolns]

The changes in this PR enable pretty print of musical score diagrams.

The changes in qualtran\drawing\_show_funcs.py aim to be enable pretty formatting in a way that is independent from internal logic (to allow for changes) but flexible (to easily adapt to any changes):

• MSD data is captured on the way to the diagram
• Inspected, re-shuffled, evaluated, and simplified using incremental RegEx and SymPy steps
• Re-inserted for final rendered.

If the internal logic changes and labels change (I estimate minimally to moderately), the approach can be adapted by modifying the foundational RegEx capture of symbols and/or the locals for simpify.

&

PR also include a small change to qualtran\bloqs\rotations\quantum_variable_rotation.py. Perhaps I missed something when I set things up since I am submitting this PR as part of unitaryHack and there is limited time to get familiar with the codebase. That said, the offset was causing errors.

Closes #853

Maybe.

Ps. I tend to use more space and comments, but I tried my best to stick to the style in the target files. Happy to expand.

[jbolns]

@mpharrigan I might be coming across a bit brisk. Apologies. I'd never done hackathons and apparently it gets to my head. :smiley: Nonetheless, I value the feedback thus far and the importance of good code. High quality outcome > fast outcome.

[ACE07-Sev]

@jbolns Run ./check/format-incremental --apply to fix your formatting issues. Should only change the 4 files that are complaining.

Also, if you don't have it set up locally, I suggest using wsl and running all the CIs locally so that when the maintainers run the CI, you'd know it's gonna pass and move directly to merge or further notes.

[jbolns]

Done, @ACE07-Sev. Thanks for the heads up. Totally missed that one.

[mhucka]

For reference, I'm copying here the image from a comment by the original author in issue #853 that shows a screenshot of the output produced by the code in this PR:

[mhucka]

@jbolns With respect to the output produced by your code currently (c.f. previous comment above), let's step back a bit and review some conventions of these "musical score diagrams":

• the horizontal axis = time, with time progressing from the left (earlier operations) to the right (later operations)
• the vertical axis = qubits, registers of qubits, or (sometimes) classical bits, with each horizontal line representing a separate qubit/register/classical bit
• boxes on the horizontal lines represent gates, bloqs, operations, or other such constructs operating on the qubit/register/etc

Now, with this in mind, comparing the original description in issue #853 where @tanujkhattar showed a screenshot illustrating the format of the diagrams, a discrepancy becomes apparent. The original always has the bloq/gate/operation name in the box, and parameters above the box:

By contrast, in your image, the entire text is inside the box:

So, this is the first thing I suggest be addressed in the PR: move the parameters or arguments (in this case, rotation angles) out and above the boxes.

[jbolns]

I'm very happy for this feedback and would like to investigate a solution for it. Having said that, I do believe it would be best to merge this PR first and then address this formatting matter as part of a new broader issue revisiting the entire MSD workflow.

The reasoning is as follows.

Currently, without anything in this PR (except the one-line fix in quantum_variable_rotation.py), Qualtran is printing the following diagrams (with full expression and all details inside the box):

If you didn't change the placement of labels intentionally, then it means the change went unnoticed. That's already the second such situation, the first being the bug that made it necessary for me to include the one-line fix in quantum_variable_rotation.py.

Things like this tend to go unnoticed when they are collateral damage from changes to other (more important) features. As such, I am in a professional obligation to consider that if I change this back on a rush, to meet the deadline of the hackathon, I might undo changes introduced for reasons more important than MSDs.

To do this appropriately, thus, I would need to take a deep dive into past commits to learn what happened and how can it be fixed without additional collateral damage.

This takes time and goes far beyond adding a simple pretty formatter.

Ergo, I suggest merging the pretty formatter in this PR and then open a broader issue for "revising the MSD workflow" with a description along the lines of "PR #1658 introduced a pretty formatter for MSDs and found a number of inconsistencies with the MSD workflow that appear to result from pre-existing changes to the codebase. It would be ideal to review the MSD workflow to ensure it is working efficiently and gives users the option to print full or pretty results using the changes introduce in 1658".

This is really only a suggestion made from my very limited point of view. It is, of course, your repository, so I will respect your choice.

Either way, I can only work more on this starting end of week.

[jbolns]

I have added a new commit. As seen in the image below, it recovers the content (though not the formatting) of labels as illustrated on the original issue.

If both the content and formatting can be recovered, prettification should be waaaaaaay easier. To begin, since the expression is already given fairly simplified, I do not need to worry about potential variation in mathematical patterns. The whole sympify situationship might not even be necessary.

Why? / What? / Where? / When? Using a combination of git bisect and git rebase (on local branches that I haven't pushed even to my fork to avoid accidents, as these are quite serious operations), I found out the following.

A. Commit 38db1aff2c4b37944acbb817350b5effffade114 broke MSDs in two ways.

• qualtran/bloqs/rotations/quantum_variable_rotation.py: Changing line 169 for line 200 causes expression not to simplify automatically, reason why labels now render the full mathematical expression.
  • I reverted back to the original line.
• qualtran/bloqs/rotations/quantum_variable_rotation.py: The original version used an integer for indexing, in lines 170-173. The changed to an offset, in line 202, causes indexing errors in the loop.
  • I had already addressed this by exchanging the offset for an integer when the offset itself is not an integer.

The fix in this PR's last commit (i.e., the one I just pushed) keeps everything in the newest version except the misbehaving lines.

To get a sense of whether this combined version could be problematic, before changing the lines in this PR, I created a test local branch to rebase the code all the way from 38db1aff2c4b37944acbb817350b5effffade114.

There was only one conflict that required solving:

• qualtran/bloqs/rotations/quantum_variable_rotation.py: In line 198 of commit 38db1aff2c4b37944acbb817350b5effffade114, there is a change from return {self.cost_reg.name: bb.join(out, self.cost_reg.dtype)} to return {self.cost_reg.name: bb.join(out, self.cost_dtype)}.
• This seemed simply a git conflict, because the latest version seems to work fine. So, I have kept the latest version.
• It is yet to be seen if this causes a mess with tests in other parts of Qualtran (I purposely skipped check/pytest so you can see yourselves if any and which tests fail).

B. Commit db953b9c48dc18146cf91e3164dfcce1207e6b61 broke the formatting of the MSDs.

• Before this commit, MSDs rendered with labels on top of the boxes with the gates, bloqs, etc constructs.
• After this commit, MSDs render only with labels containing mathematical expression inside the box.

It is a significant commit, so I will explore it at a different point in time.

@mhucka

[jbolns]

I checked on B (last comment) and found what I think is a somewhat smart solution.

The key was realising that the current behaviour in main is not the desired/ideal point of departure.

For the same reason, the final solution is different to anything in this PR.

For the same reason, I made a separate PR, #1669 .

For the same reason, this PR can be closed.

@mhucka