-
Notifications
You must be signed in to change notification settings - Fork 29
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
Compatibility with qutip v5 #116
Conversation
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.
src/krotov/objectives.py
Outdated
@@ -15,6 +15,11 @@ | |||
|
|||
import numpy as np | |||
import qutip | |||
from packaging.version import parse as parse_version | |||
if parse_version(qutip.__version__) < parse_version("5"): | |||
is_qutip5 = False |
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 going to go into krotov v2.0
, and I think it would be fine to make qutip v5.0
a requirement for that release. So I don't think this is needed. We can drop compatibility with QuTiP 4
28e1367
to
e9754de
Compare
src/krotov/propagators.py
Outdated
return qutip.Qobj( | ||
unstack_columns(self._y), | ||
dims=state.dims, | ||
isherm=True, | ||
dtype="csr" | ||
) |
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.
dtype
is not a parameter of Qobj
.
Is that output really sparse?
If the output need to be converted, use to
:
return qutip.Qobj( | |
unstack_columns(self._y), | |
dims=state.dims, | |
isherm=True, | |
dtype="csr" | |
) | |
return qutip.Qobj( | |
unstack_columns(self._y), | |
dims=state.dims, | |
isherm=True, | |
).to("CSR") |
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.
Just as a note (and this could be a separate PR): The sparse storage of QuTiP objects was probably a major source of slowness in krotov. Most usages of this package would be for small Hilbert spaces – certainly all the examples are for small Hilbert spaces. If I followed QuTiP's development correctly, version 5 should have the ability to use dense storage internally, and it would be good to use that as much as possible here (in the examples). Of course, if people manually define their objectives with sparse objects, those should still be respected.
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.
Yes, with qutip v5 you can do things all dense. One should set the qutip default as dense matrix in krotov and use sparse only if required by users.
BTW, this function seems not tested, since the mistake Eric mentioned doesn't lead to any falling tests. Maybe they are in the notebooks you mentioned?
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.
Quite possibly… the notebooks definitely serve as "integration tests". With my update of the tooling, you should be able to run it locally and/or the CI should run it for you.
src/krotov/propagators.py
Outdated
@@ -266,11 +280,17 @@ def _rhs(t, rho, L_list): | |||
out = np.zeros(rho.shape[0], dtype=complex) | |||
L = L_list[0][0] | |||
L_coeff = L_list[0][1] | |||
spmvpy_csr(L.data, L.indices, L.indptr, rho, L_coeff, out) | |||
if is_qutip5: | |||
out = L_coeff * L @ rho |
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.
rho
and out
are numpy arrays that need to be wrapped.
out = L_coeff * L @ rho | |
rho = qutip.data.Dense(rho, copy=False) | |
out = qutip.data.matmul(L, rho, scale=L_coeff) |
With return out.as_ndarray()
at the end.
src/krotov/propagators.py
Outdated
for n in range(1, len(L_list)): | ||
L = L_list[n][0] | ||
L_coeff = L_list[n][1] | ||
spmvpy_csr(L.data, L.indices, L.indptr, rho, L_coeff, out) | ||
if is_qutip5: | ||
out = L_coeff * L @ rho |
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.
spmvpy_csr
has an implicit out +=
that was forgotten.
out = L_coeff * L @ rho | |
out = qutip.data.add(qutip.data.matmul(L, rho, scale=L_coeff), out) |
We have imatmul_data_dense(left, right, scale, out)
that does the matmul implace like spmvpy_csr
, but no python binding. (It's only used in QobjEvo
.)
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.
If I keep things in qutip.Qobj
, not NumPy array, since we have a much better initialization of Qobj
now in v5. Will sparse_csr_qobj * dense_ket_qobj
automatically use that imatmul_data_dense
?
This will allow the switch between dense and csr.
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.
Since the states, (out, rho) are used by scipy's ODE solver they must enter and exit as numpy's array.
imatmul_data_dense
is equivalent to out += left @ right * scale
, this can't be represented in Qobj.__matmul__
operation so no. QobjEvo.matmul_data(t, state, out)
would use the inplace operation if out is given and dense. But this use the list format directly and rewriting to use QobjEvo
is probably not a small task.
qutip.data.add
and qutip.data.matmul
use qutip's data that can be sparse, dense, jax etc. so it should enough to be able to switch between format.
I've updated the tooling for the So, the current
I've added comments in the test file to explain where these files come from, in case they need to be updated. Basically, they're copied from the example notebooks (for one of them, you'd have to temporarily add a
There's probably also still the example notebooks, and for one of them, the |
I've released |
@BoxiLi I rebased your PR branch on the latest Maybe you can update your own |
Sounds good! If you run into any technical problems, let me know. You can also hit me up on the Unitary Fund Discord or the Julia Slack if there's something that would benefit from real-time discussion, or we can always hop on a Zoom call or something |
189663f
to
3743055
Compare
@goerz All the main tests are passing now on Linux, I think I'll merge to |
The overhead in the new The overhead for using the ODE solver is still there, unfortunately, for now. |
Very nice! And that's even without the further potential improvements from #119 (right?), which I think will make a big difference. Let me know if you need any help or input with wrapping this up for final review and merging. |
Indeed, that is not considered, but one has to use a simple self-written expm propagator, not the default krotov propagators to bypass that. Ok! I'll merge this to the development branch |
This is a fix to make krotov compatible with qutip version 5.
oct_result.dump
with the result from the first Jupyter Notebook example.dense2D_to_fastcsr_fmode
andspmvpy_csr
. For the latter one, I simply multiply the data layer matrix to the vector and hope the dispatch works.1e-7
to5e-4
intests/test_objectives.py
. I think it is just machine precision but I'm not entirely sure.oct_result_incomplete.py
for the serialization test, which is now the only test failing on my machine.