-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QubitizationWalkOperator
add attribute: sum of LCU coefficients
#890
QubitizationWalkOperator
add attribute: sum of LCU coefficients
#890
Conversation
@@ -64,6 +67,7 @@ class QubitizationWalkOperator(GateWithRegisters): | |||
|
|||
select: SelectOracle | |||
prepare: PrepareOracle | |||
sum_of_ham_coeffs: Optional[float] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value here should be 1 or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't set a default to avoid accidentally forgetting it while building walk ops
But as the sum_of_ham_coeffs
attribute is not needed in the decomposition/call-graph, it won't break anything. It's only needed for Hamiltonian simulation, in which case it needs to be correctly calculated anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I think Optional[float] = None in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be better to not have a default, and let the user explicitly specify None
when they don't know the value? (just to make sure it's not accidentally omitted)
It's typically called lambda in the literature which is problematic in python as a variable name I know. Maybe sum_of_lcu_coeffs? |
Is |
c8c6269
to
aff2de8
Compare
Yes I think that makes sense. That way we can also implicitly compute this value because we usually pass the exact coeffs to Prepare (which are normalized there). For ex. it would simplify this (we can drop L93) Qualtran/qualtran/bloqs/chemistry/ising/hamiltonian.py Lines 92 to 94 in 91b80b1
|
cc39c78
to
cde48eb
Compare
@tanujkhattar PTAL |
bd58405
to
d84fde2
Compare
cf873a9
to
1d1612f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits
@@ -92,6 +94,11 @@ def signature(self) -> Signature: | |||
def reflect(self) -> ReflectionUsingPrepare: | |||
return ReflectionUsingPrepare(self.prepare, control_val=self.control_val, global_phase=-1) | |||
|
|||
@cached_property | |||
def sum_of_lcu_coefficients(self) -> Optional[float]: | |||
r"""value of $\lambda$, i.e. sum of the coefficients $w_l$.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clarify by writing "sum of absolute values of coefficients. " instead. In this case, the docstring of the class does clarify that each
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
qualtran/bloqs/select_and_prepare.py
Outdated
a selection register $|i\rangle$. In order to prepare such a state, the PREPARE circuit is also | ||
allowed to use a junk register that is entangled with selection register. | ||
the coefficients as amplitudes of a state $|\Psi\rangle = \sum_{i=0}^{N-1} \sqrt{\frac{c_{i}}{\lambda}} |i\rangle$ | ||
where $\lambda = \sum_i c_i$, using a selection register $|i\rangle$. In order to prepare such |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where $\lambda = \sum_i c_i$, using a selection register $|i\rangle$. In order to prepare such | |
where $\lambda = \sum_i | c_i |$, using a selection register $|i\rangle$. In order to prepare such |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
qualtran/bloqs/select_and_prepare.py
Outdated
|
||
@property | ||
def l1_norm_of_coeffs(self) -> Optional['SymbolicFloat']: | ||
r"""Sum of the coefficients $c_i$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r"""Sum of the coefficients $c_i$. | |
r"""Sum of the absolute values of coefficients $c_i$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
fixes #868
The sum of coefficients determines the complexity of hamiltonian simulation. This will also enable us to propagate symbolic values to the prepare gate.
@tanujkhattar could you review this? Also it would be great if someone with qchem expertise checks whether the sum of coeffs for the Hubbard gate is correct.