Skip to content
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

Fix tlist QobjEvo with constant c_ops in mesolve #1561

Merged
merged 3 commits into from May 27, 2021

Conversation

jakelishman
Copy link
Member

@jakelishman jakelishman commented May 27, 2021

Previously, passing a Hamiltonian to mesolve which used "array" time dependence with a tlist not equal to mesolve's would fail if any constant collapse operators were present. This was because mesolve was a little over-zealous in promoting c_ops to time-dependent QobjEvo using the mesolve time list. This would cause an unnecessary failure if the Hamiltonian was a QobjEvo specified previously with a different tlist.

Fix #1560

Changelog

  • Specialise exception types in QobjEvo failure paths
  • Fix tlist QobjEvo with constant collapse operators in mesolve
  • 2-element list elements in QobjEvo specifiers can now be any iterable type, not just list

Several exceptions in QobjEvo were just raising Exception rather than
raising a specific error.  This made it more difficult for user code to
catch allowable failings in their own code, and made it harder to
determine what the problem had been.
Previously, passing a Hamiltonian to mesolve which used "array" time
dependence with a tlist different to mesolve's would fail if any c_ops
were present.  This was because mesolve would make time-dependent
QobjEvo instances out of every collapse operator, whether or not it was
actually time-dependent, using the tlist passed to mesolve.  This was a
problem if the interpolation for the Hamiltonian needed to be finer
grained than the evaluation points of the integration, because the
tlists would be incompatible when combining all c_ops into one.

See qutipgh-1560
@coveralls
Copy link

coveralls commented May 27, 2021

Coverage Status

Coverage increased (+0.02%) to 64.919% when pulling 00a7dbd on jakelishman:fix-tlist-c_ops into b21966b on qutip:master.

@jakelishman
Copy link
Member Author

I've just pushed an extra commit purely to keep CodeClimate happy - I had to touch a line in a logic block that CodeClimate already didn't like, so it moaned that I hadn't fixed it. This just separates out the logic to make it a bit easier to read. As a side-effect, it allows iterable type to be used inside the time-dependent object list of QobjEvo (only - doesn't affect anywhere that doesn't use QobjEvo), so

qutip.QobjEvo([qutip.sigmax(), (qutip.sigmay(), '1')])

is now a valid specifier (note the tuple in the list).

@jakelishman jakelishman added this to the QuTiP 4.6.2 milestone May 27, 2021
@jakelishman jakelishman changed the title Fix tlist QobJEvo with constant c_ops in mesolve Fix tlist QobjEvo with constant c_ops in mesolve May 27, 2021
Isn't intended to change behaviour, just to tidy up the format checks to
keep CodeClimate happier.  The one behavioural change is that in a
time-dependent list like
    [H1, [H2, td2], [H3, td3], ...]
the inner lists of two can now be any iterable type (e.g. tuple), which
is closer to what was intended anyway.
return [self._td_op_type(element) for element in Q_object] or -1
raise TypeError("Incorrect Q_object specification")

def _td_op_type(self, element):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot more readable, thank you.

raise TypeError("Incorrect Q_object specification")
if isinstance(td, Cubic_Spline):
out = 4
elif callable(td):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't test the validity of the function in __init__ because of the dynamics_args: passed args could not be usable before parsing them with _args_checks. Do you think that testing them if _args_checks did no work would be worth it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, to be honest - I didn't think about it at all here. I only changed these parts of QobjEvo because I needed CodeClimate to be quiet while I made an unrelated change. At least in the 4.x branch, I think what we've got is fine. I think it'll be much easier to be rigorous in the 5.x branch with each term individually gaining its own Coefficient.

I didn't even want to make this change, since it's just one more thing to merge up into dev.major, but oh well...

I think that right now we shouldn't bother testing functions for validity - with the way dynamic arguments are handled in QobjEvo right now, it would probably involve some rather difficult logic to organise that, and I really wouldn't be convinced we'd get all the edge cases. We can't catch every possible error a function could give anyway, so perhaps we should only attempt tests that are convenient for us to do? I suppose we have to have some level of trust that the user will pass something usable.

@jakelishman jakelishman merged commit acb6cfe into qutip:master May 27, 2021
@jakelishman jakelishman deleted the fix-tlist-c_ops branch May 27, 2021 17:18
quantshah pushed a commit to quantshah/qutip that referenced this pull request Jun 2, 2021
Fix tlist QobjEvo with constant c_ops in mesolve
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tlist are not compatible when using collapse operators in mesolve
3 participants