Use dispatchers to allow Qobj to use any backing data #1351

jakelishman · 2020-08-25T15:26:38Z

Finally, Qobj can use any backing data store. This fairly small and simple PR is effectively the culmination of all the work on dev.major over the last few months, and effectively is just like flicking a switch to turn on all the capabilities that we've built up through #1282, #1296, #1332, #1338 and #1341.

There is still a lot of work to be done, but I'm trying to transition to smaller, easier-to-check PRs to make review easier.

Possibly incomplete to do list:

write a proper data.create
possibly add the dispatchers in as data.Data mathematical methods (e.g. __matmul__)
add options for controlling default output types from the dispatchers
tests for Qobj with both types
tests for creation and property routines of the data layer
tests for the dispatch operation
tests for conversion operations
more specialisations to be written for Dense and CSR/Dense
more ergonomic selection of method, not just output type (for example, you should be able to specify you want the Dense specialisation of eigs even if you pass it a CSR)
more ergonomic dispatchers for matrix creation (e.g. how exactly will qutip.basis function, and how will users add specialisations?)
documentation, both user-facing and developer-facing
fix algorithms in add_csr and matmul_csr to use csr.Accumulator (should provide a speedup and remove some sorts)
fix isherm_csr (see Incorrect results from zcsr_isherm #1350 - isherm_csr uses the same algorithm, so has the same problem)

jakelishman · 2020-08-25T15:30:11Z

@Ericgig if you want to merge #1342 first, I'll rebase this PR on top of it once it's in.

coveralls · 2020-08-25T15:54:48Z

Coverage increased (+0.006%) to 64.793% when pulling 11195f9 on jakelishman:core-use-dispatchers into 3fdd9e2 on qutip:dev.major.

Ericgig · 2020-08-25T17:11:43Z

@jakelishman I merged #1342, you can rebase.
Will this PR be merge ready once the merge conflict resolved?

These are specifically for matrices which are used in base constructions, and in places such as `Qobj`. Operators in `core/operators.py` and the like should not get their own _data-layer_ dispatchers, but should define a dispatcher in their own file.

The type conversion of the output goes the other way!

Just for improving self-documentation, no meaningful changes.

Comparison function for checking something is a zero matrix. This may lead in the future to an `isequal` function for direct comparisons between two matrices.

To achieve this, we have to swap the order of `scale` and `out` in the call structure, because the dispatchers need to have a "compatible" signature, and they can't achieve this if `left, right, scale` are passed in positional form. This can possibly cause problems in Cython code; preliminary testing seemed to suggest that Cython would do something wrong if I passed the last parameter as a keyword (out) but not the penultimate one (scale), which still had a default. I was doing this in a period when there were other problems, however, so this may not have been the exact cause.

In previous versions of this function (before the fast_csr_matrix/CSR switchover) the old C-struct CSR_Matrix would be resized if enough elements had been deleted. Since our CSR type may be backed by numpy storage, this isn't as clearly a safe operation any more. This may be a solveable problem, but right now it's not a high priority as the downsides to _not_ doing it are not large.

With QuTiP's default settings, this is one of the most important functions to define to make sure that `Qobj` really uses a new data type, since many mathematical operations will implicitly include a call to `data.tidyup`. Without a specialisation for it, this will end up in the data getting converted to a different type immediately.

The most obvious change is that `ptrace` now does something sensible when faced with an `operator-ket` or `operator-bra` (it swaps back to matrix form for the operator, performs the operation, and reverts), rather than just loudly failing. The documentation is now written for what happens when given a superoperator, but the behaviour is unchanged from how QuTiP has generally handled them.

jakelishman · 2020-08-25T17:33:58Z

I suppose the answer is "yes", but there's still that huge to do list at the top that needs to be handled (including some really really important bits like the tests). I'm just rebasing it now.

By the way, I'm getting a lot of build directories dropped in whatever my current working directory is whenever I run anything which uses the new coefficients (and there's the import warning still).

jakelishman · 2020-08-25T17:47:47Z

That's the rebase done. Those tests should pass now; they do on my machine.

Ericgig · 2020-08-25T17:56:14Z

Which warnings? pyximport's? This is still used by brmesolve.
I forgot about the build directory... Will see to it later. Thank you for remindering me.

jakelishman · 2020-08-25T18:02:27Z

jake@tauros$ python
Python 3.7.7 (default, May  6 2020, 04:59:01)
[Clang 4.0.1 (tags/RELEASE_401/final)] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import qutip
/Users/jake/.anaconda3/anaconda3/envs/qutip-dev/lib/python3.7/site-packages/setuptools/distutils_patch.py:26: UserWarning: Distutils was imported before Setuptools. This usage is discouraged and may exhibit undesirable behaviors or errors. Please use Setuptools' objects directly or at least import Setuptools first.
  "Distutils was imported before Setuptools. This usage is discouraged "
>>>

It was caused by the original coefficients PR. Could be interplay between pyximport and the later import of setuptools, but I'm not sure (definitely the import of setuptools in core/coefficient.py is what actually triggers the warning, but I'm not sure when distutils is imported). I don't think you need setuptools (which is also causing the build directories) - I think you can use importlib to import the result of cythonize directly?

At any rate, it would be very good to drop the pyximport dependency from BR too.

Ericgig · 2020-08-25T18:42:41Z

qutip/tests/core/test_qobjevo.py

+        pytest.param('spline',
+                     [[cplx_qobj, cplx_coeff.spline(tlist, args)], cte_qobj],
+                     {}, id='spline'),
+        pytest.param('array', [cte_qobj,


If you prefer this spacing, at least be consistent.

oh, that was a mistake. I didn't even mean to change the first bit

or more, I didn't mean to commit it - I just fiddle around with it while I'm reading it

I'll revert it and fix the commit

Mostly this is a simple change; we just remove the `_csr` or `_csr_dense_dense` (or whatever) suffix from the data operations to make them dispatching. In a few places of old code (like in steadystate) we manually force the data types into the CSR format so the rest of the code can continue unimpeded. In theory, this now means that QuTiP can use any data-layer type for every single operation, and mix them completely freely. In reality, there may be a couple of places where CSR format is assumed in ways I forgot to check for, and certainly the creation of objects still _strongly_ favours creating CSR types. The ergonomics of creating Qobjs and selecting dispatch methods (such as for expm) where it may be beneficial to convert the input into another type still need to be improved.

Ericgig

Qobj's backing going through the Dispatcher means your work is online with this. Great!
Do we merge this like this or wait for more specializations (isherm_dense, tidyup with rtol, etc.)
I would prefer the data layer to be more hidden: not ask the user to access it for feature previously available. But I don't have clean solution to propose.

Ericgig · 2020-08-25T19:02:05Z

qutip/core/qobj.py

        if np.any(np.imag(out) > settings.core['atol']) or not self.isherm:
            return out
        else:
            return np.real(out)

    @_tidyup
-    def expm(self, method='dense'):
+    def expm(self, dtype=_data.Dense):


Why ask for the data type here, instead of the string? This force the user to know where to find the data layer object, which I don't like... But the string are messy and cannot be extended as easily to new types... I don't like it but it has it's advantages.

Yeah, I don't like this either. It's a similar problem with eigs, but that's even less obvious. I'm going to think about it a bit more, and hopefully I'll have some more ideas to suggest - it's one of my to do bullet points.

Ericgig · 2020-08-25T19:04:51Z

qutip/core/superop_reps.py

-_SINGLE_QUBIT_PAULI_BASIS = (identity(2), sigmax(), sigmay(), sigmaz())
+# TODO: revisit when creation routines have dispatching.
+_SINGLE_QUBIT_PAULI_BASIS = (
+    identity(2).to(_data.CSR),


Wouldn't it be better to have these as Dense per default?

Possibly? They're only used in the next function down in superop_reps, and I didn't test enough of what it was doing to know if a sparse or dense representation was better for that function. It looks sort of sparse, maybe?

jakelishman · 2020-08-25T19:28:59Z

I'm happy to go either way on whether to merge this now or later. I'm happy to keep adding to it, but I'm also mindful that I don't want this to turn into another +15000/-15000 PR.

I possibly need to look into that test failure a little more - it's temperamental, and I'm not sure if it's something where it's just a case of a too strict tolerance, or if there is an actually numerical precision problem.

Ericgig · 2020-08-25T19:35:40Z

I prefer smaller PRs, this is big enough.
Continue in another one please.
I will put the test to a to check list and review it when working on brmesolve.

jakelishman · 2020-08-25T19:41:48Z

It's ~caused by the swap to the data-layer - probably it's just minor floating-point uncertainties due to slightly changed algorithms, but I'm not 100% sure on that.

jakelishman requested review from ajgpitch and Ericgig and removed request for ajgpitch August 25, 2020 15:27

jakelishman added 10 commits August 25, 2020 18:16

Improve warning in Dispatcher

0185778

Add constant-matrix dispatchers

43fca5f

These are specifically for matrices which are used in base constructions, and in places such as `Qobj`. Operators in `core/operators.py` and the like should not get their own _data-layer_ dispatchers, but should define a dispatcher in their own file.

Fix dispatch weighting with output

afde542

The type conversion of the output goes the other way!

Swap csr.from_coo_pointers variable names

37a864a

Just for improving self-documentation, no meaningful changes.

Add iszero function

ff39eed

Comparison function for checking something is a zero matrix. This may lead in the future to an `isequal` function for direct comparisons between two matrices.

Add link to referenced lecture notes

df634d5

jakelishman force-pushed the core-use-dispatchers branch from f7d6f7f to 44f36d9 Compare August 25, 2020 17:46

Ericgig reviewed Aug 25, 2020

View reviewed changes

jakelishman force-pushed the core-use-dispatchers branch from 44f36d9 to 11195f9 Compare August 25, 2020 18:50

Ericgig approved these changes Aug 25, 2020

View reviewed changes

Ericgig merged commit 6d59722 into qutip:dev.major Aug 25, 2020

jakelishman deleted the core-use-dispatchers branch August 26, 2020 10:42

jakelishman mentioned this pull request Mar 29, 2021

Qobj with dense data array #437

Closed

jakelishman added this to the QuTiP 5.0 milestone Jun 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dispatchers to allow Qobj to use any backing data #1351

Use dispatchers to allow Qobj to use any backing data #1351

jakelishman commented Aug 25, 2020 •

edited

Loading

jakelishman commented Aug 25, 2020

coveralls commented Aug 25, 2020 •

edited

Loading

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020 •

edited

Loading

jakelishman commented Aug 25, 2020

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020 •

edited

Loading

Ericgig Aug 25, 2020

jakelishman Aug 25, 2020

jakelishman Aug 25, 2020

jakelishman Aug 25, 2020

Ericgig left a comment

Ericgig Aug 25, 2020

jakelishman Aug 25, 2020

Ericgig Aug 25, 2020

jakelishman Aug 25, 2020

jakelishman commented Aug 25, 2020

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020

Use dispatchers to allow Qobj to use any backing data #1351

Use dispatchers to allow Qobj to use any backing data #1351

Conversation

jakelishman commented Aug 25, 2020 • edited Loading

jakelishman commented Aug 25, 2020

coveralls commented Aug 25, 2020 • edited Loading

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020 • edited Loading

jakelishman commented Aug 25, 2020

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericgig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman commented Aug 25, 2020

Ericgig commented Aug 25, 2020

jakelishman commented Aug 25, 2020

jakelishman commented Aug 25, 2020 •

edited

Loading

coveralls commented Aug 25, 2020 •

edited

Loading

jakelishman commented Aug 25, 2020 •

edited

Loading

jakelishman commented Aug 25, 2020 •

edited

Loading