Repository navigation
Accumulate ParallelLinearQubitOperator matvecs in-place (#1410) - #1450
Conversation
…antumlib#1410) Accumulate partial matvecs into one buffer with in-place += instead of functools.reduce(numpy.add), for both the single-process and multiprocess paths. Defer forkserver setup until processes > 1 so the single-process path works on Windows.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes memory usage and improves platform compatibility for ParallelLinearQubitOperator. It replaces functools.reduce(numpy.add, ...) with a new helper function _accumulate_vectors that performs in-place vector accumulation to avoid intermediate allocations. Additionally, it ensures that the 'forkserver' multiprocessing start method is only set when spawning multiple processes, allowing the single-process path to run on platforms like Windows. Unit tests have been added to cover the new helper function and the single-process execution path. There are no review comments, so I have no feedback to provide.
mhucka
left a comment
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this contribution. If you could address a couple of minor items, I think it'll be ready to go after that.
| if not ParallelLinearQubitOperator._start_method_set: | ||
| # Only required when actually spawning workers; the single-process path | ||
| # must remain usable on platforms without forkserver (e.g. Windows). | ||
| if self.options.processes > 1 and not ParallelLinearQubitOperator._start_method_set: |
There was a problem hiding this comment.
If the caller passes in an options object that has no processes field or processes = None, this will result in an error. Can you add a defensive check or wrap it in a try-except clause?
Maybe something like
processes = getattr(self.options, 'processes', None) or 1
and then use processes instead of self.options.processes.
| ``functools.reduce(numpy.add, ...)`` builds a new full-sized array for every | ||
| partial sum. For large state vectors that creates substantial temporary | ||
| memory pressure. Accumulating with in-place ``+=`` keeps a single result | ||
| buffer instead. |
There was a problem hiding this comment.
Please switch the use of double backquotes to single backquotes in the docstring.
Use getattr for options.processes with a single-process fallback, and switch the helper docstring to single backticks.
|
Thanks @mhucka, both addressed, lmk how this looks. |
2 participants
Implements the suggestion from #1410: avoid
functools.reduce(numpy.add, …)when combining partial matvecs, which was allocating a new full-sized array for every partial sum.Used the approach from GCA and went a bit further:
+=accumulation for both the single-process and multiprocess paths (same cost when combining worker results).forkserverwhenprocesses > 1, so the single-process path works on Windows.There is one nuance with
processes=1, where operator grouping usually yields a single group so the biggest + is on the multiprocess reduce. Happy to trim or adjust if its better to keep the change scoped only to the single-process path.Tests passed, and will wait for CI to see if I missed anything.
Fixes #1410