QREF integration #1194

mstechly · 2024-07-25T20:03:07Z

I added qref_interop module to Qualtran.

Allows to convert Bloqs into QREF objects
Includes demo of how to use it with Bartiq

There's a couple of issues with this PR:

module is called qref_interop, but it's actually not "interop" but "one-way conversion" at this moment. Should we rename it or leave it as is?
I've written tests using get_bloq_examples from dev_tools. However, this is outside from Qualtran package, so not a way to go. I like the functionality though, so perhaps it's worth moving it to the Qualtran package?
Failing tests are either skipping (failure on qualtran side) or xfailing (failure on QREF side). I found some recurring issues with certain bloqs, will post them in separate issue/issues. Since this is very much beta, I didn't want those tests to fail yet. Not sure if you agree with this approach.
I'm not sure if the notebook demo is compelling enough, but decided I'll share this version and get your feedback first.
As we discussed I tried analyzing call graph and getting some information from there, but we'd need to add some features in QREF to actually make it useful, so it's not even included in this PR.

google-cla · 2024-07-25T20:03:12Z

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.

mpharrigan · 2024-07-26T17:18:22Z

Awesome! This will be great to have in the repository! Thanks so much for opening a PR. I suspect we may have a couple nits before merging, but this looks like a great start.

We'll have to add qref to the dependencies. For better or for worse, we've been putting our interop dependencies alongside the ordinary runtime dependencies (maybe we'll factor these out into optional dependencies in the future) in dev_tools/requirements/deps/runtime.txt.

tanujkhattar

Thanks for opening the PR! The conversion already looks great!

I've left a round of comments and suggestions.

qualtran/qref_interop/_bloq_to_qref.py

tanujkhattar · 2024-07-27T07:27:54Z

qualtran/qref_interop/_bloq_to_qref.py

+        A dictionary that can be unpacked into arguments of RoutineV1 initializer.
+    """
+    ports = [port for reg in bloq.signature for port in _ports_from_register(reg)]
+    local_variables = {}


Can you add a comment explaining what's the use case for local_variables ? It seems like it may be used when the string representation of the symbolic size of a qualtran register does not satisfy the isIdentifier() condition which is needed for any string to be a valid port size in QREF ?

After some thoughts I decided this logic should actually go to bartiq, will update bartiq and then remove it from here.

Actually, it would require some major changes in bartiq and if done like this it should work, so I'll leave it as it is.
Added a comment.

An extra question – is there a way to get all the symbols used in a particular bloq?
In most cases the symbols taken from signature are enough, but sometimes (e.g. in PermutationCycle), I don't know how to retrieve the information that it requires having L defined, other than parsing the t_complexity of it.

We don't have an interface that does exactly this. We often try to treat symbols and concrete values as similarly as possible in the code (this was our design decision; it has its plus-es and minus-es). In practice, this means symbols get passed in as constructor arguments (aka bloq attributes). But they can be arbitrarily hidden:

N, L = sympy.symbols("N L", positive=True, integer=True) cycle = Shaped((L,)) permutation_cycle_symb = PermutationCycle(N, cycle)

the L symbol is passed as a constructor argument, but contained within a Shaped object (which we use to support cases where the concrete value is an array but we only need the shape to do e.g. resource counts).

Since everything is an attrs dataclass, you can extract the bloq attributes with (pseudocode)

fields = attrs.fields(bloq) vals = {f.name: getattrs(bloq, f.name) for f in fields}

so you can see that the vals dictionary contains {'N': sympy.Symbol('N') (easy), and 'cycle': Shaped(sympy.Symbol('L'))} (you have to poke at it to see that it has a symbol named L

qualtran/qref_interop/_bloq_to_qref.py

tanujkhattar · 2024-07-27T07:44:42Z

qualtran/qref_interop/_bloq_to_qref.py

+def bloq_to_routine(obj, name: Optional[str] = None):
+    """Import object from Qualtran by converting it into corresponding QREF RoutineV1 object.
+
+    Args:
+        obj: object to be imported. Can be either Bloq or BloqInstance.
+        name: optional name override. This can be useful e.g. if you are converting
+            CompositeBloq (which do not store meaningful names in Qualtran) and
+            know some good name for it.
+
+    Return:
+        A Bartiq object corresponding to the source Qualtran object. For both Bloqs
+        and BloqInstances the returned object is of type RoutineV1.
+
+    """


If I understand correctly, as part of the conversion; we are not doing any bloq decompositions and the expectation is that the user should flatten out a bloq that has a decomposition into a CompositeBloq at an appropriate level and then convert the CompositeBloq to qref's SchemaV1; the conversion would not have any further information about the structure of the subbloqs (eg: info about decomposing a bloq that appears in the composite bloq but is not decomposed). Is this understanding correct?

Yes, that's correct.
If there is a preferred way to perform the decomposition down to arbitrary level, we could add an option to perform it while doing the conversion. However, I felt this would be an overkill for the first iteration and it's better to be explicit with what's being converted for now.

sounds good, I think this is fine as long as we're clear that a RoutineV1 is analogous to one CompositeBloq

qualtran/qref_interop/bartiq_demo.ipynb

tanujkhattar · 2024-07-27T07:52:30Z

qualtran/qref_interop/bartiq_demo.ipynb

+      "/Users/mstechly/Documents/code/2024-06-26-qualtran-conversion/bartiq/src/bartiq/compilation/_compile.py:117: UserWarning: Found the following issues with the provided routine after the compilation has finished: [\"Input param N found in subroutine: CompositeBloq.PrepareUniformSuperposition_0, which is not among top level params: {'PrepareUniformSuperposition_0.N'}.\"]\n",
+      "  warnings.warn(\n"


We usually clear the output of all notebooks before committing them. The notebooks are run before generating the docs so that the docs generated on https://qualtran.readthedocs.io/en/latest/index.html have the expected output.

Can you please clear all output from the notebook (assuming the notebooks runs after qualtran runtime dependencies are updated)

tanujkhattar · 2024-07-27T07:53:39Z

qualtran/qref_interop/_bloq_to_qref.py

@@ -0,0 +1,311 @@
+import networkx as nx


nit: I think it's worth splitting up this file into smaller files with methods grouped based on the function they are performing. But happy to merge it as a single file for now and leave the reorganization as a future improvement.

I'd prefer to leave it for reorganization in future.

qualtran/qref_interop/test_bloq_to_qref.py

tanujkhattar · 2024-07-27T07:59:59Z

Also, I think it's worth adding qref to dev_tools/requirements/deps/runtime.txt in a separate PR and then run dev_tools/requirements/re-pip-compile-in-docker.sh to recompile the .env.txt files.

mstechly · 2024-07-29T20:08:56Z

@tanujkhattar I'll do as you suggested with the separate PR for the dependencies once this PR is clean and I make any necessary changes in qref/bartiq (there's at least one right now).

…ran into mstechly/qref-integration

tanujkhattar · 2024-08-01T17:08:53Z

qualtran/qref_interop/_bloq_to_qref_test.py

@@ -0,0 +1,261 @@
+from dev_tools.qualtran_dev_tools.bloq_finder import get_bloq_examples


@mpharrigan This is not part of the qualtran package; so we should probably not use it in tests to avoid adding a dependency.

Yeah, @mstechly called it out in the original PR description. For the tests, I'd recommend picking a few key examples to test the qref interop functionality and then later we can add

a qref conversion check to the "bloq report card" which is run ad-hoc

a qref conversion check to the bloq_autotester pytest fixture, which will run against most bloq examples

Ok, I've chosen some examples (open to using other examples) and changed the code.

mstechly

I think I have addressed all the feedback, let me know what you think.

qualtran/qref_interop/_bloq_to_qref.py

mstechly · 2024-08-08T18:29:54Z

qualtran/qref_interop/_bloq_to_qref_test.py

@@ -0,0 +1,261 @@
+from dev_tools.qualtran_dev_tools.bloq_finder import get_bloq_examples


Ok, I've chosen some examples (open to using other examples) and changed the code.

qualtran/qref_interop/bartiq_demo.ipynb

mstechly · 2024-08-13T21:21:31Z

@tanujkhattar @mpharrigan – I've signed the CLA and ran all the checks locally to make sure they're passing.

Apart from the outstanding discussion, some other comments:

I'm importing private methods in _bloq_to_qref_test.py like this: from qualtran.bloqs.arithmetic.addition import _add_oop_large. If there's a better way to import it, please let me know.
As @mpharrigan noticed, the version of QREF and Bartiq I suggested were incompatible – I made a new PR to update versions in Qualtran: Update dependencies for newer qref & bartiq versions #1284
Until this PR is merged, the notebooks check might be failing, but I hope others will work.

Hopefully we can close it soon 😅

mpharrigan · 2024-08-14T20:34:23Z

I'm importing private methods in _bloq_to_qref_test.py like this: from qualtran.bloqs.arithmetic.addition import _add_oop_large. If there's a better way to import it, please let me know.

This is fine, especially from a test file; and because this is a bloq example, so it's not "private" in the traditional sense.

I'll merge the version bump PR, take a final look over this, and we should be able to merge imminently!

mpharrigan

looks great! Really nice notebook

comments were addressed

mstechly added 4 commits July 25, 2024 19:08

WiP QREF integration.

eb7356a

Conversion to QREF updated

325d8a3

remove outdated import

88843b8

Add notebook with Bartiq interop demo

41cc523

mpharrigan requested review from mpharrigan and tanujkhattar July 26, 2024 17:18

tanujkhattar previously requested changes Jul 27, 2024

View reviewed changes

minor refactor and fixes

1df9dbd

mstechly added 4 commits July 29, 2024 22:09

Merge branch 'main' into mstechly/qref-integration

d1f2db3

Fixes in QREF conversion

4d688eb

Update notebook output

8db252d

Merge branch 'mstechly/qref-integration' of github.com:mstechly/Qualt…

8eebf63

…ran into mstechly/qref-integration

mstechly mentioned this pull request Jul 31, 2024

TComplexity not working for several bloqs #1225

Closed

Update tests

2907d67

tanujkhattar reviewed Aug 1, 2024

View reviewed changes

Further fixes in QREF integration

83019c8

mstechly commented Aug 8, 2024

View reviewed changes

mstechly added 2 commits August 8, 2024 20:36

Merge branch 'main' into mstechly/qref-integration

3f35f80

fix outdated import

5e3b961

mstechly mentioned this pull request Aug 8, 2024

Add qref and bartiq dependencies #1267

Closed

mpharrigan and others added 5 commits August 9, 2024 12:43

Merge branch 'main' into mstechly/qref-integration

ed88cb0

Merge branch 'main' into mstechly/qref-integration

1dda49f

fix formatting

cc68225

fix style issues

bad1366

Merge branch 'main' into mstechly/qref-integration

6186e9e

Merge branch 'main' into mstechly/qref-integration

6331a1b

mpharrigan approved these changes Aug 14, 2024

View reviewed changes

Merge branch 'main' into mstechly/qref-integration

95e3c66

mpharrigan merged commit 184d9d3 into quantumlib:main Aug 14, 2024
8 checks passed

mstechly mentioned this pull request Aug 26, 2024

Rename params & variables PsiQ/qref#115

Open

mstechly mentioned this pull request Sep 3, 2024

Qualtran conversion PsiQ/qref#7

Closed

mstechly deleted the mstechly/qref-integration branch September 16, 2024 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QREF integration #1194

QREF integration #1194

mstechly commented Jul 25, 2024 •

edited

Loading

google-cla bot commented Jul 25, 2024

mpharrigan commented Jul 26, 2024

tanujkhattar left a comment

tanujkhattar Jul 27, 2024

mstechly Jul 29, 2024

mstechly Jul 30, 2024

mstechly Jul 30, 2024

mpharrigan Aug 14, 2024

tanujkhattar Jul 27, 2024

mstechly Jul 29, 2024

mpharrigan Aug 14, 2024

tanujkhattar Jul 27, 2024

tanujkhattar Jul 27, 2024

mstechly Jul 29, 2024

tanujkhattar commented Jul 27, 2024

mstechly commented Jul 29, 2024

tanujkhattar Aug 1, 2024

mpharrigan Aug 5, 2024

mstechly Aug 8, 2024

mstechly left a comment

mstechly Aug 8, 2024

mstechly commented Aug 13, 2024

mpharrigan commented Aug 14, 2024

mpharrigan left a comment

		"/Users/mstechly/Documents/code/2024-06-26-qualtran-conversion/bartiq/src/bartiq/compilation/_compile.py:117: UserWarning: Found the following issues with the provided routine after the compilation has finished: [\"Input param N found in subroutine: CompositeBloq.PrepareUniformSuperposition_0, which is not among top level params: {'PrepareUniformSuperposition_0.N'}.\"]\n",
		" warnings.warn(\n"

		@@ -0,0 +1,261 @@
		from dev_tools.qualtran_dev_tools.bloq_finder import get_bloq_examples

QREF integration #1194

QREF integration #1194

Conversation

mstechly commented Jul 25, 2024 • edited Loading

google-cla bot commented Jul 25, 2024

mpharrigan commented Jul 26, 2024

tanujkhattar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujkhattar commented Jul 27, 2024

mstechly commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstechly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstechly commented Aug 13, 2024

mpharrigan commented Aug 14, 2024

mpharrigan left a comment

Choose a reason for hiding this comment

mstechly commented Jul 25, 2024 •

edited

Loading