Feat: added MultiOperator and multiproc/multithread support to matmat/rmatmat - #780
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 1 high |
🟢 Metrics -26 complexity · -5 duplication
Metric Results Complexity -26 Duplication -5
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@cako if you have time, appreciate any feedback on this 😄 The initial motivation was to support multiproc/threading in |
cako
left a comment
cako
left a comment
There was a problem hiding this comment.
Overall looks pretty good, I just left a couple of suggestions!
| ylocal = op(x).squeeze() | ||
| with lock: | ||
| y[:] += ylocal |
There was a problem hiding this comment.
I know this logic already existed but I can see this becoming a bottleneck for large parallelization. I think a better strategy would be to let the multioperator return its local result and the main thread/process can finish summing. Because at that point its a single assessor writing, there's no need for a lock.
There was a problem hiding this comment.
Interesting!
I have a feeling that what you suggest was my initial approach (basically copy-paste from multiprocess) but then I tried this one for threading and paid off... but I can't find any proper performance test I did with the two approaches.
Let me write Github Issue about this and I will try both options with a proper comparison - #786
| def _matmat_multithread(self, X: NDArray, pool: ThreadPoolExecutor) -> NDArray: | ||
| """Matrix-matrix multiplication (multithreaded version)""" | ||
| ys = list( | ||
| pool.map( | ||
| lambda args: _matvec_rmatvec_map(*args), | ||
| [(self._matvec, col.reshape(-1)) for col in X.T], | ||
| ) | ||
| ) | ||
| y = np.vstack(ys).T | ||
| return y | ||
|
|
||
| def _matmat_multiproc(self, X: NDArray, pool: Pool) -> NDArray: | ||
| """Matrix-matrix multiplication (multiprocess version)""" | ||
| ys = pool.starmap( | ||
| _matvec_rmatvec_map, | ||
| [(self._matvec, col.reshape(-1)) for col in X.T], | ||
| ) | ||
| y = np.vstack(ys).T | ||
| return y |
There was a problem hiding this comment.
Is this part missing sp.sparse.issparse(X) special treatment?
There was a problem hiding this comment.
Ops, I may have forgotten 😢 let me add it (and add a test!)
2 participants
This PR aims to extend the multiproc/multithread capabilities in PyLops.
It implements:
MultiOperatorfor all operators with multiproc/multithread support (adopted byBlockDiag,HStack,VStackKroneckerKroneckerwith multiproc/multithreadSome examples and timings for the new multiproc/multithread support in matmat/rmatmat and Kronecker are in https://github.com/PyLops/pylops_notebooks/blob/master/developement/Basic_multi.ipynb