Skip to content

Use greedy_topological_sort to sort operations in musical scores - #1948

Open
Acciaccatura wants to merge 13 commits into
quantumlib:mainfrom
Acciaccatura:issue1640
Open

Use greedy_topological_sort to sort operations in musical scores#1948
Acciaccatura wants to merge 13 commits into
quantumlib:mainfrom
Acciaccatura:issue1640

Conversation

@Acciaccatura

@Acciaccatura Acciaccatura commented Sep 5, 2026

Copy link
Copy Markdown

substitutes nx.topological_generations for greedy_topological_sort when ordering operations in musical score layouts. additionally:

  • creates appropriate topological generations taking into account the fact that allocs, or right-register operations in general, may reuse (and hence have a temporal dependence on) free operations.
  • as the motivation for this change seems to be to match up with qubit counts (see Improve qubit counting logic to minimize qubit counts over multiple allocations #1636), adds a test case to ensure musical score data lines up with expected qubit count.
    • the implementation of the test case may depend on how the musical score generator formats its output, so I'm happy to find alternative ways to structure this test, or toss it altogether :^)

output ends up looking like this (though it should be noted this PR leaves the original display logic intact and still does not make use of topo_gen. i assume we would like to make use of that in a separate PR meant for cleaning up the musical scores?):

bloq = TestManyAlloc(5, False)
msd = get_musical_score_data(bloq.decompose_bloq())
fig, ax = draw_musical_score(msd)
fig.show()
qualtran_issue_1640_output

fixes #1640

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the musical score drawing logic in qualtran/drawing/musical_score.py to use a greedy topological sort and adds a test verifying that the musical score aligns with the qubit count. However, a critical bug was identified where the logic for updating topological generations and incrementing seq_x is incorrectly indented outside of the main loop, leading to rendering issues and incorrect topological generation assignments. A detailed code suggestion was provided to fix the indentation and logic.

Comment thread qualtran/drawing/musical_score.py
@Acciaccatura
Acciaccatura marked this pull request as draft September 6, 2026 08:40
mainly, this actually sets topo_gen to a value but also better implements the desired logic for
setting (and possibly later updating) topological generations.

this also fixes some erroneous line indentations.
@Acciaccatura
Acciaccatura marked this pull request as ready for review September 7, 2026 21:39

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the musical score drawing logic to use a greedy topological sort and track the depth of each qubit line to compute topological generations. It also adds a test to verify that the musical score aligns with the expected qubit count. Feedback on these changes includes addressing a potential KeyError when looking up newly allocated lines in y_to_topo_gen and removing a leftover assert False debugging statement in the new test.

Comment thread qualtran/drawing/musical_score.py
Comment thread qualtran/drawing/musical_score_test.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

greedy_topological_sort in musical score

1 participant