Adding function to create a quirk url from a bloq#1821
Adding function to create a quirk url from a bloq#1821Axel-pappalardo wants to merge 4 commits intoquantumlib:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
| def test_bloq_to_quirk(): | ||
| bloq_to_quirk(Add(QUInt(5))) | ||
| bloq_to_quirk(MultiTargetCNOT(5)) | ||
|
|
||
|
|
||
| def test_bloq_to_quirk_on_atomic(): | ||
| bloq_to_quirk(Toffoli()) |
There was a problem hiding this comment.
can we add more robust testing than this?
There was a problem hiding this comment.
I added more testing, hope this is satisfactory, testing is complicated as quirk url as quite long and pasting long url is something I prefer to avoid
…eManager + more testing
| @pytest.mark.parametrize("n", range(3, 6)) | ||
| def test_composite_bloq_to_quirk_url_shape(n): | ||
| cbloq = MultiTargetCNOT(n).decompose_bloq().flatten() | ||
| url = composite_bloq_to_quirk(cbloq) | ||
|
|
||
| assert url.startswith('https://algassert.com/quirk#circuit={"cols":[') | ||
| assert url.endswith(']}') | ||
|
|
||
|
|
||
| def test_bloq_to_quirk(): | ||
| url_add = bloq_to_quirk(Add(QUInt(5))) | ||
| assert url_add.startswith('https://algassert.com/quirk#circuit={"cols":[') | ||
| assert url_add.endswith(']}') | ||
| url_mtcnot = bloq_to_quirk(MultiTargetCNOT(3)) | ||
| assert ( | ||
| url_mtcnot | ||
| == 'https://algassert.com/quirk#circuit={"cols":[[1,"•",1,"X"],[1,"•","X",1],["•","X",1,1],[1,"•","X",1],[1,"•",1,"X"]]}' | ||
| ) | ||
|
|
||
|
|
||
| def test_negate_to_quirk(): | ||
| url = bloq_to_quirk(Negate(QUInt(2))) | ||
| assert ( | ||
| url | ||
| == 'https://algassert.com/quirk#circuit={"cols":[["X",1,1,1,1],[1,"X",1,1,1],[1,1,1,"X",1],[1,"•",1,"•","X"],["X",1,1,1,"•"],[1,"•",1,"•","X"],["X",1,"•",1,1],[1,"X",1,"•",1],[1,1,1,"X",1]]}' | ||
| ) |
There was a problem hiding this comment.
these tests would break if we change the implementation details of MultiTargetCNOT, Add, Negate, ...
Can you just wire up a simple circuit with bloq builder for testing? then the actual circuit can be small too and the url won't be huge
|
/gemini review |
There was a problem hiding this comment.
Code Review
This PR adds a new feature to generate Quirk URLs from bloqs. The implementation is mostly correct, but there are several critical issues in SparseLineManager that could lead to runtime errors due to incorrect handling of DanglingT instances. I've provided suggestions to fix these. Additionally, there are opportunities to improve code quality and portability in composite_bloq_to_quirk by refactoring the line removal logic and using the webbrowser module instead of a hardcoded subprocess call.
| dual_candidates = [ | ||
| (rpos.seq_x, soq.binst.i) # type: ignore[union-attr] | ||
| for soq, rpos in self.soq_assign.items() | ||
| if rpos.y == line and rpos.seq_x > start and soq.binst.bloq_is(dual_cls) | ||
| ] |
There was a problem hiding this comment.
The soq.binst attribute can be a DanglingT instance, which does not have bloq_is or i attributes. Accessing them will lead to a runtime AttributeError. You should add a check to ensure soq.binst is a BloqInstance before accessing these attributes.
| dual_candidates = [ | |
| (rpos.seq_x, soq.binst.i) # type: ignore[union-attr] | |
| for soq, rpos in self.soq_assign.items() | |
| if rpos.y == line and rpos.seq_x > start and soq.binst.bloq_is(dual_cls) | |
| ] | |
| from qualtran import BloqInstance | |
| dual_candidates = [ | |
| (rpos.seq_x, soq.binst.i) | |
| for soq, rpos in self.soq_assign.items() | |
| if isinstance(soq.binst, BloqInstance) | |
| and rpos.y == line | |
| and rpos.seq_x > start | |
| and soq.binst.bloq_is(dual_cls) | |
| ] |
| join_to_split = {} | ||
| for soq, rpos in self.soq_assign.items(): | ||
| if soq.binst.bloq_is(Join) and soq.idx == (): | ||
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Split) | ||
| if dual_id is not None: | ||
| join_to_split[soq.binst.i] = dual_id # type: ignore[union-attr] | ||
| return join_to_split |
There was a problem hiding this comment.
The soq.binst attribute can be a DanglingT instance, which does not have bloq_is or i attributes. Accessing them will lead to a runtime AttributeError. You should add a check to ensure soq.binst is a BloqInstance before accessing these attributes.
| join_to_split = {} | |
| for soq, rpos in self.soq_assign.items(): | |
| if soq.binst.bloq_is(Join) and soq.idx == (): | |
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Split) | |
| if dual_id is not None: | |
| join_to_split[soq.binst.i] = dual_id # type: ignore[union-attr] | |
| return join_to_split | |
| from qualtran import BloqInstance | |
| join_to_split = {} | |
| for soq, rpos in self.soq_assign.items(): | |
| if isinstance(soq.binst, BloqInstance) and soq.binst.bloq_is(Join) and soq.idx == (): | |
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Split) | |
| if dual_id is not None: | |
| join_to_split[soq.binst.i] = dual_id | |
| return join_to_split |
| split_to_join = {} | ||
| for soq, rpos in self.soq_assign.items(): | ||
| if soq.binst.bloq_is(Split) and soq.idx != (): | ||
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Join) | ||
| if dual_id is not None: | ||
| split_to_join[soq.binst.i] = dual_id # type: ignore[union-attr] | ||
| return split_to_join |
There was a problem hiding this comment.
The soq.binst attribute can be a DanglingT instance, which does not have bloq_is or i attributes. Accessing them will lead to a runtime AttributeError. You should add a check to ensure soq.binst is a BloqInstance before accessing these attributes.
| split_to_join = {} | |
| for soq, rpos in self.soq_assign.items(): | |
| if soq.binst.bloq_is(Split) and soq.idx != (): | |
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Join) | |
| if dual_id is not None: | |
| split_to_join[soq.binst.i] = dual_id # type: ignore[union-attr] | |
| return split_to_join | |
| from qualtran import BloqInstance | |
| split_to_join = {} | |
| for soq, rpos in self.soq_assign.items(): | |
| if isinstance(soq.binst, BloqInstance) and soq.binst.bloq_is(Split) and soq.idx != (): | |
| dual_id = self._find_dual_on_line(rpos.y, rpos.seq_x, Join) | |
| if dual_id is not None: | |
| split_to_join[soq.binst.i] = dual_id | |
| return split_to_join |
| def maybe_reserve(self, binst, reg, idx): | ||
| # Reserve one slot so a partitioned wire can reclaim the same vertical region | ||
| # at its dual Join/Split. | ||
| if binst.bloq_is(Join) and reg.shape: | ||
| dual_id = self._join_to_split_id.get(binst.i) | ||
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) | ||
|
|
||
| if binst.bloq_is(Split) and not reg.shape: | ||
| dual_id = self._split_to_join_id.get(binst.i) | ||
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) |
There was a problem hiding this comment.
The binst argument can be a DanglingT instance, which does not have a bloq_is method. Calling it will lead to a runtime AttributeError. You should add a check to ensure binst is a BloqInstance before calling this method.
| def maybe_reserve(self, binst, reg, idx): | |
| # Reserve one slot so a partitioned wire can reclaim the same vertical region | |
| # at its dual Join/Split. | |
| if binst.bloq_is(Join) and reg.shape: | |
| dual_id = self._join_to_split_id.get(binst.i) | |
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) | |
| if binst.bloq_is(Split) and not reg.shape: | |
| dual_id = self._split_to_join_id.get(binst.i) | |
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) | |
| def maybe_reserve(self, binst, reg, idx): | |
| from qualtran import BloqInstance | |
| # Reserve one slot so a partitioned wire can reclaim the same vertical region | |
| # at its dual Join/Split. | |
| if isinstance(binst, BloqInstance) and binst.bloq_is(Join) and reg.shape: | |
| dual_id = self._join_to_split_id.get(binst.i) | |
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) | |
| if isinstance(binst, BloqInstance) and binst.bloq_is(Split) and not reg.shape: | |
| dual_id = self._split_to_join_id.get(binst.i) | |
| self.reserve_n(1, lambda binst_to_check, reg_to_check: binst_to_check.i == dual_id) |
| nb_deleted_lines = 0 | ||
| for i in range( | ||
| msd.max_y + 1 | ||
| ): # deleting lines of the circuit which are not used (happens with partition) | ||
| ind = i - nb_deleted_lines | ||
| for col in circuit: | ||
| line_is_useless = col[ind] == '1' | ||
| if not line_is_useless: | ||
| break | ||
| if line_is_useless: | ||
| for col in circuit: | ||
| col.pop(ind) | ||
| nb_deleted_lines += 1 |
There was a problem hiding this comment.
The current implementation for removing unused lines is hard to read and inefficient due to modifying lists while iterating over them. A more Pythonic and efficient approach would be to first identify the lines to keep and then construct a new circuit. This improves both readability and performance.
| nb_deleted_lines = 0 | |
| for i in range( | |
| msd.max_y + 1 | |
| ): # deleting lines of the circuit which are not used (happens with partition) | |
| ind = i - nb_deleted_lines | |
| for col in circuit: | |
| line_is_useless = col[ind] == '1' | |
| if not line_is_useless: | |
| break | |
| if line_is_useless: | |
| for col in circuit: | |
| col.pop(ind) | |
| nb_deleted_lines += 1 | |
| # deleting lines of the circuit which are not used (happens with partition) | |
| if circuit: | |
| num_lines = len(circuit[0]) | |
| lines_to_keep = [i for i in range(num_lines) if any(col[i] != '1' for col in circuit)] | |
| circuit = [[col[i] for i in lines_to_keep] for col in circuit] |
| if open_quirk: | ||
| subprocess.run(["firefox", url], check=False) |
There was a problem hiding this comment.
Using subprocess.run with a hardcoded "firefox" is not portable. The webbrowser module is the standard Python way to open a URL in the user's default browser, which is more portable and robust.
| if open_quirk: | |
| subprocess.run(["firefox", url], check=False) | |
| if open_quirk: | |
| import webbrowser | |
| webbrowser.open(url) |
As per the title, This adds a function which yields a quirk url from a qualtran bloq.
The function isn't perfect, and runs into many limitation:
handled_operationsdict), operations not handled are simply ignored.It works by using the musical score of the bloq to look through the operations column per column and thus can use a
LineManager.I also implemented a default
LineManagerSparseLineManagerto avoid an issue where if 2 different qubit lines share the same horizontal line in a musical score (which can happen withPartition) they would be put on the same line in Quirk (this is because to determine Quirk coordinates for an operation we only look at its coordinates in the musical score).SparseLineManageravoid this issue by reserving the slots used by any line that ends after aPartitionand freeing them only if the adjoint of thatPartitionappears again. (for example, if I join 3 QBit into a QAny(3), the slots used by the 3 QBit are reserved until I split the QAny(3) back into 3 QBit).An example generated with that code:
Lookup table
It could still have some issue with certain Bloq, but making specific
LineManagershould helpLet me know what you think