qutip icon indicating copy to clipboard operation
qutip copied to clipboard

Inconsisten naming between mesolve and propagator.

Open AGaliciaMartinez opened this issue 3 years ago • 8 comments

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.

AGaliciaMartinez avatar Nov 18 '21 16:11 AGaliciaMartinez

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".

AGaliciaMartinez avatar Nov 18 '21 16:11 AGaliciaMartinez

Hello, I would like to attempt this issue. It will be my first QuTip contribution.

RobHam99 avatar Nov 19 '21 18:11 RobHam99

@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.

hodgestar avatar Nov 19 '21 22:11 hodgestar

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.

BoxiLi avatar Nov 20 '21 09:11 BoxiLi

So I should find another issue to work on?

RobHam99 avatar Nov 20 '21 10:11 RobHam99

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.

Sounds sensible enough to me! I would also go with this solution instead of the one I proposed.

AGaliciaMartinez avatar Nov 21 '21 11:11 AGaliciaMartinez

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

RobHam99 avatar Nov 21 '21 12:11 RobHam99

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?

AGaliciaMartinez avatar Nov 29 '21 10:11 AGaliciaMartinez

Renamed to c_ops in v5.

Ericgig avatar Apr 01 '24 05:04 Ericgig