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

Speed up the HEOM RHS construct #2128

Merged
merged 18 commits into from Mar 22, 2023

Conversation

hodgestar
Copy link
Contributor

@hodgestar hodgestar commented Mar 20, 2023

Description

Adds a _from_csr_blocks constructor to speed up the construction of the HEOM RHS by a factor of ~4x.

What allows from_csr_blocks to be fast is that we can order the blocks in a way that makes it possible to write the rows of the output one after the other without a lot of work, and we never have to add a new non-zero element into a row or make a copy of the big output matrix.

The implementation in Python was slow just because there is a lot of Python looping and arithmetic when the number of operators being combined becomes large.

_from_csr_blocks is a private constructor for now. If use cases emerge we can consider making something similar part of the public interface later.

Related issues or PRs

  • None

@hodgestar
Copy link
Contributor Author

@nwlambert Would you mind giving this a try with some bigger realistic HEOM examples you have lying around? Eric's recent improvements to CSR mul and imul also help HEOM RHS construction since there is a lot of c * op happening. The 3.5x improvement from this branch is on top of that.

@Ericgig Would you mind giving your thoughts on the Cython interface both from a "is this good from a technical point of view" and from a "do we want this point of view". I guess we could also name it _from_csr_blocks if we don't want to expose it.

@Ericgig
Copy link
Member

Ericgig commented Mar 20, 2023

It doesn't look robust enough to be user facing as it is, but as a private function, if you get a 3.5x, I'd say we want it.

Technically, just give a type to i and it's all running in c code.

@coveralls
Copy link

coveralls commented Mar 20, 2023

Coverage Status

Coverage: 75.328% (-0.06%) from 75.391% when pulling 58f82aa on hodgestar:feature/faster-heom-rhs-construction into 17e7958 on qutip:master.

@hodgestar
Copy link
Contributor Author

It doesn't look robust enough to be user facing as it is, but as a private function, if you get a 3.5x, I'd say we want it.

I'll rename it to _from_csr_blocks and add some more checks on the ordering and shape of the ops if those aren't too expensive and add some tests. We can expose it publicly if that's ever useful.

Technically, just give a type to i and it's all running in c code.

Thanks! Adding the type to i did make it a little faster.

@hodgestar hodgestar requested a review from Ericgig March 22, 2023 11:47
@hodgestar hodgestar changed the title WIP: Add Cython from_csr_blocks method to speed up the HEOM RHS construct Speed up the HEOM RHS construct Mar 22, 2023
@hodgestar hodgestar added this to the QuTiP 5.0 milestone Mar 22, 2023
Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Seems fine.
This fixes the used data type to CSR and this should be documented.
I think you want csr.zeros instead of csr.empty.

qutip/core/data/csr.pyx Outdated Show resolved Hide resolved
qutip/tests/core/data/test_csr.py Outdated Show resolved Hide resolved
@hodgestar
Copy link
Contributor Author

This fixes the used data type to CSR and this should be documented.

It is documented in the docstring for block_ops? Or did you want more documentation somewhere?

@hodgestar
Copy link
Contributor Author

@Ericgig Thank you for the review. Would you mind having a last look? I made quite a few small changes.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

Better with zeros.

I would document in HEOMSolver say there that the Hamiltonian and bath operator will be coerced to csr.

@hodgestar hodgestar merged commit 775cdcc into qutip:master Mar 22, 2023
11 checks passed
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.

None yet

3 participants