Remove _t_complexity_ if build_call_graph exists #1044
Conversation
- If call graph exists, then the t complexity is redundant. These t_complexities are moved to tests to ensure that the t_complexity is as expected. - Only issue is LPRSInterimPrep whose _t_complexity_ is not consistent.
| def test_lp_resource_state_symb(): | ||
| bloq = _lp_resource_state_symbolic.make() | ||
| assert bloq.t_complexity().t == 4 * bloq.bitsize | ||
|
|
||
|
|
There was a problem hiding this comment.
perhaps we should keep this test marked @pytest.mark.xfail? (so that we don't forget to fix symbolics for LPRS.)
There was a problem hiding this comment.
It is not ideal to have the symbolis not working -- maybe it's best to wait till we can add a CRz Bloq and use that so we can continue to support symbolics.
I don't think there's any rush to remove t_complexity; especially by breaking symbolic resource counting which is a lot more useful.
There was a problem hiding this comment.
Reverted this and reenabled symbolics.
| @@ -94,9 +93,6 @@ def build_call_graph(self, ssa: 'SympySymbolAllocator') -> Set['BloqCountT']: | |||
| } | |||
| return ret | |||
|
|
|||
| def _t_complexity_(self) -> 'TComplexity': | |||
| return TComplexity(rotations=self.bitsize + 1, clifford=2 + self.bitsize) | |||
There was a problem hiding this comment.
@tanujkhattar isn't this TComplexity expression wrong? we use self.bitsize CRz gates but TComplexity.rotations is only for single qubit rotations, right?
There was a problem hiding this comment.
Yes, this is totally wrong. Even if we revert the change to fix symbolics, this expression still needs to change.
There was a problem hiding this comment.
We often use rotations in TComplexity to count up both 1 and 2 qubit rotations (though ideally it should be just 1 qubit phase rotations). Another example is CZPowGate -
rotations=1. This is actually fine since CZPowGate has a decomposition where we can use only 1 ZPow rotation + 2 Toffoli gates.
Eventually, I think these fundamental 1 and 2 qubit rotation gates should be leaf nodes that we count up and user can choose to how to decompose them; which has a constant additive or small multiplicative overhead.
Tl;Dr - I'm fine with updating the expression now but we should add a comment explaining why we have the constant overhead of 2.
There was a problem hiding this comment.
Reverted the t_complexity removal, and added a comment. Let me know if I misrepresented the factor in the comment.
There was a problem hiding this comment.
maybe we should make this change in a different PR? because it's a bug-fix for the tcomplexity expression, unlike the other changes that remove _t_complexity_
There was a problem hiding this comment.
I don't think that's necessary
| # Uses self.bitsize controlled-Rz rotations which decomposes into | ||
| # 2 single-qubit rotations and 3 cliffords |
If call graph exists, then the t complexity is redundant. These t_complexities are moved to tests to ensure that the t_complexity is as expected.
Only issue is LPRSInterimPrep no longer supports symbolics for t_complexity.
Fixes: #905