qutip
qutip copied to clipboard
Inconsisten naming between mesolve and propagator.
Describe the issue
Currently mesolve
and propagator
have different names for the same argument. mesolve
uses c_ops
whereas propagator
uses c_op_list
. This can lead to confusion (as seen in the google group discussion). It would probably be best to be consistent with the naming.
A possible solution
We could deprecate c_op_list
and use c_ops
instead (or the other way around). We could also raise a deprecation warning:
if 'c_op_list' in kwargs:
warnings.warn("c_op_list argument name has been deprecated in favor of c_ops for consistency in the library. This will be remover in future versions.",
category=DeprecationWarning)
c_ops = c_op_list
To be honest, it looks like kwargs
in mesolve
should not be an option. It is only used to retrieve the num_cpus
input from it. I would suggest deprecating it too and just add a new argument, num_cpus
that is properly documented.
To Reproduce
a = qutip.destroy(10)
H = a.dag() * a
c_ops = [a]
psi0 = qutip.coherent(10, 0.1)
t_list = np.linspace(0, 25.0, 1001)
# This works
result = qutip.mesolve(H, psi0, t_list, c_ops=c_ops, e_ops=[a])
# This works and can be used to reproduce the above result. Note the kwarg is c_ops_list instead
# of c_ops as in mesolve.
U = qutip.propagator(H, t_list, c_op_list=c_ops)
# This does not work as intended but it gets executed. That is because c_ops in interpreted as being part of kwargs.
# The biggest issue is that no error is raised.
U1 = qutip.propagator(H, t_list, c_ops=c_ops)
Expected behavior
Functions in QuTiP should use the same name for the argument c_ops
.
Your Environment
QuTiP: Quantum Toolbox in Python
================================
Copyright (c) QuTiP team 2011 and later.
Current admin team: Alexander Pitchford, Nathan Shammah, Shahnawaz Ahmed, Neill Lambert, Eric Giguère, Boxi Li, Jake Lishman and Simon Cross.
Board members: Daniel Burgarth, Robert Johansson, Anton F. Kockum, Franco Nori and Will Zeng.
Original developers: R. J. Johansson & P. D. Nation.
Previous lead developers: Chris Granade & A. Grimsmo.
Currently developed through wide collaboration. See https://github.com/qutip for details.
QuTiP Version: 4.7.0.dev0+5c73300
Numpy Version: 1.21.2
Scipy Version: 1.7.1
Cython Version: 0.29.23
Matplotlib Version: 3.3.4
Python Version: 3.9.5
Number of CPUs: 4
BLAS Info: OPENBLAS
OPENMP Installed: False
INTEL MKL Ext: False
Platform Info: Linux (x86_64)
Installation path: /home/kaladin/Documents/git/qutip/qutip4/qutip
================================================================================
Please cite QuTiP in your publication.
================================================================================
For your convenience a bibtex reference can be easily generated using `qutip.cite()`
Additional context See https://groups.google.com/g/qutip/c/GPm8e702HN4 for the full discussion. There may also be other functions in QuTiP that suffer from the same inconsistency problem.
This should probably be labelled as "code" instead of bug. As mentioned in the google group, this may be labelled as a "good first issue".
Hello, I would like to attempt this issue. It will be my first QuTip contribution.
@AGaliciaMartinez @RobHam99 I will put in a small vote that we leave sorting this out for QuTiP 5 (i.e. the dev.major) branch, where I suspect it has been sorted out already.
The only QuTiP release it could go into before 5 would be 4.7 and I'd like to avoid doing too many minor clean-ups in 4.7 because I want 4.7 to just work nicely for existing users before the big just to 5. Happy to consider exceptions to that policy on a case by case basis though.
We could try adding a small fix like giving a warning if c_ops
is used in propagator
, not changing the signatures. If it is c_op_list
in propagator
then just let it be so in 4.7, and fix it only in dev.major
if needed. In this way, all existing code will still work in 4.7 and those who are still using 4.7 won't get confused again.
So I should find another issue to work on?
We could try adding a small fix like giving a warning if
c_ops
is used inpropagator
, not changing the signatures. If it isc_op_list
inpropagator
then just let it be so in 4.7, and fix it only indev.major
if needed. In this way, all existing code will still work in 4.7 and those who are still using 4.7 won't get confused again.
Sounds sensible enough to me! I would also go with this solution instead of the one I proposed.
To clarify, you want me to implement something like this message:
if 'c_ops' in kwargs:
warnings.warn("please use c_op_list argument in current version. c_op_list will be deprecated in favour of c_ops in future versions.")
c_ops = c_op_list
but not change 'c_op_list' to 'c_ops'? Then you want me to change it in the dev.major branch?
Thanks
Hey @RobHam99. I have talked today with @hodgestar and we agreed that, since **kwargs
is only used to pass the argument num_cpus
, we think it is best to raise an error (TypeError
) when kwargs contains a key that is not num_cpus
. This should hopefully avoid the same confusion to happen again with propagator, but we should definitely try to get the naming consistent for the new QuTiP version that is under active development.
@RobHam99, are you still interested in opening a Pull Request for this issue?
Renamed to c_ops
in v5.