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.

[mpharrigan]

what is the reason for this change? (add a comment?)

[jbolns]

I noted this in the issue as a concern/question.

The instructions in the issue do not lead to completion. There is an indexing error in one of the iterations in the loop below this line I added.

qvr_zpow = QvrZPow(Register('x', QFxp(12, 6, False)), gamma = sympy.Symbol('Y'))
show_bloq(qvr_zpow, 'dtype')
show_bloq(qvr_zpow.decompose_bloq(), 'musical_score')

Perhaps I'm missing a step or something. Else, it is a pre-existing bug that might be related to the small angle rotations changes in 38db1af

In the next commit, this line will move to inside the loop to minimise its involvement to only when strictly necessary.

for i in range(num_rotations):
    power_of_two = i - self.num_frac_rotations
    exp = (2**power_of_two) * self.gamma * 2
    offset = offset if isinstance(offset, int) else 1  # offset -> 1 if not int to avoid indexing errors  #HERE
    out[-(i + offset)] = bb.add(ZPowGate(exponent=exp, eps=eps), q=out[-(i + offset)])
return {self.cost_reg.name: bb.join(out, self.cost_dtype)}

However, this is only to ensure things proceed as needed for testing of the feature in this issue.

If the instructions to produce the foundational diagram were incomplete, please let me know.

If it is a bug, it should be checked separately.

[mpharrigan]

this function should live in musical_score.py

[jbolns]

Moving it. I can also use a smaller function for each soq rather than the entire msd.
Will show in next commit.
Will also add a test in the corresponding file (now possible since its a soq-level function).

[mpharrigan]

explain what transformations are done

[jbolns]

Will do. Shows in next commit.

[mpharrigan]

I'm not sure what this code aims to do, but it seems rather brittle. Why are we replacing parts of the label with the character "1"?

[jbolns]

Right, yes! You are completely right.

So, any errors fallback to the original label, which ensures completion, but I can make it more robust in the next commit.

More importantly, the mathematical concern is undeniably valid.

It's clear I took the diagram examples in the issue too literally, perhaps assuming there was some quantum magic happening somewhere that escaped me.

Thinking about it as a standard symbolic equation, there seems to be a need to define a preference:

• semi-pretty labels: some simplification is possible if Abs(Y) is not evaluated with some sort of substitution (which would affect other Y values), but the pretty version won't be entirely pretty. (better than nothing I guess)
• arbitrary replacement: pi, 2*pi, some random value? (no likey, not symbolic anymore)
• user-input placeholder: user chooses what value to use for evaluation (same)
• a panel of diagrams: several rotation angles rendered as a panel of diagrams rather than a single panel diagram (quite a deviation from original concept so I'm hoping there is another approach I'm not considering but, somewhat applicable).

I'll work a little on the next commit but it would be cool if you could tell me your views on the matter and potential alternatives not considered, to avoid the need for many additional commits.

[jbolns]

Actually, after running a few tests if seems the combination of factors is such that only very very very small angles would lead to a difference, in which case, might as well use pi to evaluate the Abs value in the expression. Will explain thoroughly in comments.

[jbolns]

@mpharrigan
I might be coming across a bit brisk. Apologies. I'd never done hackathons and apparently it gets to my head. 😃
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 38db1af 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 38db1af.

There was only one conflict that required solving:

• qualtran/bloqs/rotations/quantum_variable_rotation.py: In line 198 of commit 38db1af, 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 db953b9 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