Remove `_t_complexity_` on `MultiAnd` and support symbolic `cvs` #1015

tanujkhattar · 2024-05-29T19:48:25Z

Part of a larger effort to better support symbolic resource estimation across bloqs. I will update the cvs parameter on other bloqs in follow-up PRs.

fdmalone · 2024-05-29T19:53:00Z

qualtran/bloqs/mcmt/and_bloq.py

@@ -260,26 +256,38 @@ class MultiAnd(Bloq):
    Args:
        cvs: A tuple of control variable settings. Each entry specifies whether that
            control line is a "positive" control (`cv[i]=1`) or a "negative" control `0`.
+            If a (symbolic) integer is passed, assumes the control values to be all 1's.


Is this subject to change / worth thinking about? I guess I haven't really thought about what symbolic controls mean.

Its easy to argue for the case where the control spec is simply all 1s; and thats mostly the case I want to update everywhere we currently expect cvs: Tuple[int, ...]

In future, we'd ideally like these bloqs to be able to take any cvs: CtrlSpec and it would be more nuanced to think about what a symbolic controlled spec would mean at that point. But for now, I just want to remove blockers for enabling symbolic resource estimation for higher level bloqs like state preparation via alias sampling / reflection etc.

One change which may be necessary is that right now, we always define the control register to be of time Register('controls', QBit(), shape=(len(self.cvs),)) whereas we'll have to change it to be of type Register('controls', QAny(len(self.cvs)), shape=()) to enable self.cvs to be symbolic. But I'm not sure what the repercussions would be

I'm not super thrilled about having one attribute named cvs that can represent either a list of control values or the number of controls.

def __init__(self, *, cvs=None, n_ctrl=None): if cvs is not Nont and n_ctrl is not None: raise ValueError() self._cvs = cvs self._n_ctrl = n_ctrl @property def cvs(): ... @property def n_ctrl(): ..

Is there a way to do it without a custom init with frozendataclasses? I can also do what I did for QROM; i.e. rename cvs -> cvs_or_n_ctrl: Union[] and then expose cvs and n_ctrl as properties.

We'll have to change the call sites which construct the bloq via keyword arguments; but that should be fine right now?

I think you can do this with attrs by prepending an underscore to the field name and setting frozen(kwargs=True)

Then it would look something like this:

@frozen class MultiAnd(Bloq): _cvs: Optional[Tuple[SymbolicInt, ...]] = field( converter=_to_tuple_or_none, kw_only=True, default=None ) _n_ctrls: Optional[HasLength] = field( converter=_to_has_length_or_none, kw_only=True, default=None ) def __attrs_post_init__(self): assert (self._cvs is None) ^ (self._n_ctrls is None)

We'll have to add this logic everywhere we use cvs: Tuple[int, ...] right now. I'm fine doing this, but there's definitely more going on in this approach than having a single Union which must not be None.

Also, the main reason for defining a new Length object is so that we can set cvs to be of type Length and len(cvs) still works.

The above approach where we maintain both n_ctrls and cvs independently everywhere wouldn't require introducing Length since n_ctrls can just be Symbolic.

At other places, we typically have a union of Shaped and numpy arrays so that object.shape works irrespective of the type of object. I think following a similar pattern here would keep things consistent.

Tl;Dr - let's maintain cvs: Union[HasLength, Tuple[int, ...]] or cvs_or_n_ctrls: Union[HasLength, Tuple[int, ...]]) ? WDYT?

oh oh oh when I first read it as Union[Sequence, Length] I was like, these are two different things. But HasLength ... yeah just one field named cvs is good

can you update the docstring as well. instead of "if a symbolic integer is passed" say "if a HasLength object"

mpharrigan · 2024-05-29T20:00:41Z

qualtran/bloqs/mcmt/and_bloq.py

+    def bitsize(self) -> SymbolicInt:
+        return self.cvs.bitsize if isinstance(self.cvs, Length) else len(self.cvs)


I still think this should be n_ctrls or similar (rather than bitsize)

mpharrigan · 2024-05-29T20:01:15Z

qualtran/bloqs/mcmt/and_bloq.py

+    @property
+    def control_values(self) -> Tuple[SymbolicInt, ...]:
+        if isinstance(self.cvs, Length):
+            raise ValueError(f"{self.cvs} is symbolic")
+        return self.cvs


I still think this should be meaningfully disambiguated. and_bloq.cvs vs and_bloq.control_values ...

maybe concrete_cvs? idk

we have is_symbolic in the symbolic utils. What's the opposite of symbolic? we should pick a word for it and use that word consistently throughout the library

I can use whatever you suggest -- I trust your expertise on the English language more than mine :)

let's go with "concrete" https://en.wiktionary.org/wiki/concrete

mpharrigan · 2024-05-29T20:02:03Z

qualtran/symbolics/types.py

+class Length:
+    """Symbolic value for an object that has a length."""
+
+    bitsize: SymbolicInt


this doesn't have to represent a bitwidth, right? it can be the length of anything. Perhaps n.

mpharrigan · 2024-05-29T20:03:53Z

qualtran/symbolics/types.py

+class Length:
+    """Symbolic value for an object that has a length."""


a length is different than something that has a length. Analogously, above we use Shaped instead of Shape. Sadly, I think the English language slightly fails us here. Maybe HasLength? HasLen?

Will change to HasLength

mpharrigan · 2024-05-29T20:04:14Z

qualtran/symbolics/types.py

@@ -46,6 +46,19 @@ def is_symbolic(self):
        return True


+@frozen
+class Length:


is this a subclass of Shaped?

mpharrigan · 2024-05-29T20:46:15Z

qualtran/bloqs/mcmt/and_bloq.py

+    if isinstance(x, sympy.Expr):
+        return Length(x)


wait, shouldn't we make users pass a HasLength object explicitly? This is how we do Shaped if I'm not mistaken

Okay that's right, I'll change it.

mpharrigan

lgtm with the changes brought up in the comments

tanujkhattar · 2024-05-29T21:42:02Z

Addressed all the comments. Also realized that its a bit annoying to have HasLength as a drop-in replacement where len(cvs) just returns a symbolic value because of the way python treats the __len__ dunder method. Explained in the docstring and added a workaround. Merging now

anurudhp · 2024-05-29T22:59:18Z

Also realized that its a bit annoying to have HasLength as a drop-in replacement where len(cvs) just returns a symbolic value because of the way python treats the __len__ dunder method.

@tanujkhattar fwiw, I faced the same issue when implementing Shaped, my workaround was to define slen:

Qualtran/qualtran/symbolics/math_funcs.py

Lines 102 to 105 in 5aabc66

    
           def slen(x: Union[Sized, Shaped]) -> SymbolicInt: 
        
               if isinstance(x, Shaped): 
        
                   return x.shape[0] 
        
               return len(x)

We should extend this by adding HasLength to the input type here.

mpharrigan · 2024-05-30T00:00:00Z

and/or make HasLength be a subclass of Shaped

Remove _t_complexity_ on MultiAnd and support symbolic cvs

49b97ab

tanujkhattar requested review from mpharrigan and fdmalone May 29, 2024 19:48

tanujkhattar assigned mpharrigan and fdmalone May 29, 2024

Fix mypy

2c8f512

fdmalone reviewed May 29, 2024

View reviewed changes

mpharrigan reviewed May 29, 2024

View reviewed changes

mpharrigan approved these changes May 29, 2024

View reviewed changes

Address comments

5dce0cd

tanujkhattar enabled auto-merge (squash) May 29, 2024 21:42

tanujkhattar added 3 commits May 29, 2024 17:45

Fix mypy

668b640

Merge branch 'main' into symbolic_cvs_multi_and

b20d9d2

update generalizer tests

e61d8bb

tanujkhattar merged commit b9429f6 into quantumlib:main May 29, 2024
7 checks passed

mpharrigan added resource_counting symbolic labels May 29, 2024

This was referenced Aug 5, 2024

Migrate t_complexity to QECGatesCost cost key #1251

Closed

MultiAnd call graph returns x gates #1253

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `_t_complexity_` on `MultiAnd` and support symbolic `cvs` #1015

Remove `_t_complexity_` on `MultiAnd` and support symbolic `cvs` #1015

tanujkhattar commented May 29, 2024

fdmalone May 29, 2024

tanujkhattar May 29, 2024

tanujkhattar May 29, 2024

mpharrigan May 29, 2024

tanujkhattar May 29, 2024 •

edited

Loading

mpharrigan May 29, 2024

tanujkhattar May 29, 2024 •

edited

Loading

tanujkhattar May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

tanujkhattar May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

tanujkhattar May 29, 2024

mpharrigan May 29, 2024

mpharrigan May 29, 2024

tanujkhattar May 29, 2024

mpharrigan left a comment

tanujkhattar commented May 29, 2024

anurudhp commented May 29, 2024

mpharrigan commented May 30, 2024

		def bitsize(self) -> SymbolicInt:
		return self.cvs.bitsize if isinstance(self.cvs, Length) else len(self.cvs)

		class Length:
		"""Symbolic value for an object that has a length."""

Remove _t_complexity_ on MultiAnd and support symbolic cvs #1015

Remove _t_complexity_ on MultiAnd and support symbolic cvs #1015

Conversation

tanujkhattar commented May 29, 2024

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 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanujkhattar May 29, 2024 • edited Loading

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

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

mpharrigan left a comment

Choose a reason for hiding this comment

tanujkhattar commented May 29, 2024

anurudhp commented May 29, 2024

mpharrigan commented May 30, 2024

Remove `_t_complexity_` on `MultiAnd` and support symbolic `cvs` #1015

Remove `_t_complexity_` on `MultiAnd` and support symbolic `cvs` #1015

tanujkhattar May 29, 2024 •

edited

Loading

tanujkhattar May 29, 2024 •

edited

Loading