Fix t_complexity for symbolic ham. sim. by gqsp example
#944
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fdmalone
reviewed
May 11, 2024
fdmalone
reviewed
May 11, 2024
fdmalone
reviewed
May 11, 2024
fdmalone
reviewed
May 11, 2024
fdmalone
reviewed
May 11, 2024
fdmalone
previously requested changes
May 11, 2024
anurudhp
force-pushed
the
fix-t-complexity-for-gqsp-examples
branch
from
c8d5711 to
278354c
Compare
|
@fdmalone @tanujkhattar ptal (notebook test failure is unrelated (#911)) |
anurudhp
force-pushed
the
fix-t-complexity-for-gqsp-examples
branch
from
c8479c7 to
e67c580
Compare
mpharrigan
reviewed
May 16, 2024
qualtran/bloqs/util_bloqs.py
Outdated
| @@ -50,8 +50,31 @@ | |||
| from qualtran.simulation.classical_sim import ClassicalValT | |||
|
|
|||
|
|
|||
| class UtilBloq(Bloq, metaclass=abc.ABCMeta): | |||
There was a problem hiding this comment.
I think this is reasonable, but please
- Rename to
BookkeepingBloq, which is more specific than "util" (a name I despise -- we can change the module later :). This also makes the properties make sense: a bookkeeping bloq is trivially controlled and does notn affect the t complexity - make private, i.e.
_BookkeepingBloq
There was a problem hiding this comment.
renamed to _BookkeepingBloq
qualtran/bloqs/util_bloqs.py
Outdated
Comment on lines
54
to
58
| """Base class for util bloqs. | ||
|
|
||
| 1. Trivial controlled - pass through the control register. | ||
| 2. Do not affect T complexity. | ||
| """ |
There was a problem hiding this comment.
grammar & style in this docstring.
There was a problem hiding this comment.
polished the docstring, how does it look now?
|
Can you update the PR description to summarize the changes present in this PR |
anurudhp
force-pushed
the
fix-t-complexity-for-gqsp-examples
branch
from
542e72b to
9fac150
Compare
mpharrigan
approved these changes
May 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
t_complexitydoesn't work for StatePreparationAliasSampling #935 (comment) - adds a simplified override for the call graph of a symbolic HamSimbyGQSP bloq._BookkeepingBloqfor utility bloqs that are used for book-keeping (i.e. cast, split/join, alloc/free)