-
Notifications
You must be signed in to change notification settings - Fork 11
Fix: Enable JOPT to support open-system optimization with TRACEDIFF fidelity #49
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
base: main
Are you sure you want to change the base?
Conversation
Hello @Akhils777 , thank you for working on the PR. Which issue are you targetting? Is it #47 ? |
yes, it is #47 |
|
||
if self._fid_type == "TRACEDIFF": | ||
diff = X - self._target | ||
# to prevent if/else in qobj.dag() and qobj.tr() | ||
diff_dag = Qobj(diff.data.adjoint(), dims=diff.dims) |
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.
Hey, one question, maybe I missed something because I wasn't following it closely enough in the last few weeks.
I understand the error message says that the dimension of diff.data.adjoint()
does not agree with dims=diff.dims
. For instance, the dimension of a two-qubit state is [[4], [1]]
, but the adjoint should be [[1], [4]]
. Here we take the adjoint, but we did not change the dimension. This caused the error. It worked before, probably just because only unitary evolution was tested, not state evolution. Therefore, I think the solution would be to update the diff.dims
.
The solution in this PR seems overcomplicated. As the original comment says, we should try to use Qobj
as much as possible and avoid manually branching according to data type. Because this introduces redundant code, and the Qobj
implementation could be more efficient. For efficiency, we should use
Qobj(diff.data.adjoint(), dims=updated_dims, copy=False)
to avoid copying. Did I miss something?
How to implement this updated_dims
? We could learn from Qobj.dag()
, use
from qutip.dimentions import Dimensions
Dimensions(self._dims[0], self._dims[1])
which defines an operator that, e.g., maps space 4 to space 1, which is indeed the bra operator.
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.
from qutip.core.dimensions import Dimensions
Dimensions(dims[0], dims[1])
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.
Maybe an alternative, easiest way is to directly do
diff_dag = diff.dag()
shouldn't this be the simplest if diff
is already a Qobj
?
@Akhils777 could you try this?
if self._fid_type == "PSU": # f_PSU (drop global phase) | ||
infid = 1 - _abs(g) # custom_jvp for abs | ||
elif self._fid_type == "SU": # f_SU (incl global phase) | ||
infid = 1 - jnp.real(g) | ||
|
||
return infid | ||
infid = 1 - _abs(g) if self._fid_type == "PSU" else 1 - jnp.real(g) |
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.
This is just reformatting or a real change of the code?
I actually prefer the original because the comments explain the meaning of PSU and SU.
As discussed earlier, perhaps all we need is an appropriately place |
Summary
This PR resolves an issue where JOPT failed to optimize open quantum systems using TRACEDIFF fidelity due to incompatibilities between JAX autodiff and Qobj data structures.
Changes Made
Results
Let me know if anything needs clarification!