Cu quantum mps #17
Cu quantum mps #17
Conversation
There was a problem hiding this comment.
To be fair, I should sit down some more time to check that all operations in the helper make sense (writing them on a piece of paper and reading the cuQuantum API). But they seem reasonable (index attribution is alright, but for more complex operations I should dedicate some more time).
The main concern I have is about the cache usage, but everything else is rather slim and sensible.
I was trying to understand if it's possible to simplify the code in contract_expectation, but I'm not sure we could do much better.
|
Reporting an issue here. If using the latest version of qibo (0.2.0), this PR (commit f59b1b0) hit an error below.
It seems that the |
A quick fix. Change |
|
I was about to check, thanks for solving it on your side :) |
|
Hi @alecandido wondering if there is any other outstanding issues with this PR? Please let me know so that we can move to the next step |
There was a problem hiding this comment.
To me it looks ok.
Sorry if it took so long for the review: I've been a bit busy (and eventually I actually forgot, thanks for the gentle reminder).
I leave some further comments for minor improvements.
About the change suggestions: pay attention (as usual) because not all the suggestions are self-contained, so check before committing them from the web interface.
| mps_tensors = [state_tensor] * num_qubits | ||
| return mps_tensors |
There was a problem hiding this comment.
It's not necessarily bad, but it is just simpler to avoid the assignment, since you're immediately returning (unless you're very motivated to give it a name, but the name will usually be very close to the function one)
| mps_tensors = [state_tensor] * num_qubits | |
| return mps_tensors | |
| return [state_tensor] * num_qubits |
| Generate the MPS with an initial state of |00...00> | ||
| """ | ||
| state_tensor = cp.asarray([1, 0], dtype=dtype).reshape(1, 2, 1) | ||
| mps_tensors = [state_tensor] * num_qubits |
There was a problem hiding this comment.
I believe CuPy arrays to be immutable, so it should not be a problem.
But try to confirm it, because, otherwise, you'd be generating many references to the same object, that might have counter-intuitive effects (that is even if you modify just one, all the others will change as well.
| """ | ||
| Generate the MPS with an initial state of |00...00> | ||
| """ |
There was a problem hiding this comment.
Sooner or later we'll start using Pydocstring, and it will check that every first line will end with a full stop.
| """ | |
| Generate the MPS with an initial state of |00...00> | |
| """ | |
| """Generate the MPS with an initial state of |00...00>.""" |
(this is just an irrelevant remark, because this PR is already pretty good, so we can improve even some details).
|
|
||
| def mps_site_right_swap(mps_tensors, i, **kwargs): | ||
| """ | ||
| Perform the swap operation between the ith and i+1th MPS tensors. |
There was a problem hiding this comment.
Tell the user that the operation will be performed in-place.
| options=kwargs.get("options", None) | ||
| ) | ||
| mps_tensors[i : i + 2] = (a, b) | ||
| return mps_tensors |
There was a problem hiding this comment.
Since the operation is in-place, you do not need to return the modified object, since it's the one passed by the user.
(and there should be only one way of doing something, so it would be confusing to have the two references around)
| self.num_qubits = circ_qibo.nqubits | ||
| self.handle = cutn.create() | ||
| self.dtype = dtype | ||
| self.mps_tensors = initial(self.num_qubits, dtype=dtype) |
There was a problem hiding this comment.
Why do you need all these assignments to self?
If possible, keep everything local (in this case local to the __init__ method).
| (i + offset, 2 * i + 1, i + 1 + offset) for i in range(num_qubits) | ||
| ] | ||
|
|
||
| def contract_norm(self, mps_tensors, options=None): |
There was a problem hiding this comment.
Just a proposal to simplify the method names: since the class is already named MPSContractionHelper, you could avoid prefixing methods with contract_ (since it is reasonable that an MPSContractionHelper.norm method will make a contraction to give you the norm).
However, it is just a proposal and definitely subject to opinions. So, feel free to keep as it is.
| The norm of the MPS. | ||
| """ | ||
| interleaved_inputs = [] | ||
| for i, o in enumerate(mps_tensors): |
There was a problem hiding this comment.
| for i, o in enumerate(mps_tensors): | |
| for i, op in enumerate(mps_tensors): |
To slightly improve the name's expresiveness, at very little expense ^^
| An ndarray-like object as the state vector. | ||
| """ | ||
| interleaved_inputs = [] | ||
| for i, o in enumerate(mps_tensors): |
There was a problem hiding this comment.
Same of above
| for i, o in enumerate(mps_tensors): | |
| for i, op in enumerate(mps_tensors): |
Hi this is to address issue #15. Added conversion of Qibo circuit form to MPS format and provided method to contract to state vector format.