Update tn #74
Update tn #74
Conversation
updates: - [github.com/asottile/pyupgrade: v3.16.0 → v3.17.0](asottile/pyupgrade@v3.16.0...v3.17.0)
updates: - [github.com/psf/black: 24.4.2 → 24.8.0](psf/black@24.4.2...24.8.0)
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
The eval.py contains highly duplicated code, and this is manifest in this PR, since the same update has been repeated over and over.
Before merging this PR, increasing even more the maintenance burden, it would be worth to refactor the eval.py file (in this, or even in another PR), to avoid incurring is such a redundant diff.
| @@ -114,6 +125,9 @@ def dense_vector_tn_MPI(qibo_circ, datatype, n_samples=8): | |||
| # Sum the partial contribution from each process on root. | |||
| result = comm.reduce(sendobj=result, op=MPI.SUM, root=root) | |||
|
|
|||
| del network | |||
There was a problem hiding this comment.
What is this for? Why do you now need it?
There was a problem hiding this comment.
Assuming it is for memory management
Deletion of a name removes the binding of that name from the local or global namespace, depending on whether the name occurs in a global statement in the same code block. If the name is unbound, a NameError exception will be raised.
https://docs.python.org/3/reference/simple_stmts.html#the-del-statement
del x doesn’t directly call x.del() — the former decrements the reference count for x by one, and the latter is only called when x’s reference count reaches zero.
https://docs.python.org/3/reference/datamodel.html#object.__del__
So, unless it is exactly documented that you should apply del network before .free_all_blocks() (and even in that case, we should make sure that the underlying .__del__() is called when you wish), it doesn't make much or no difference wrt just waiting for the return, since the network name would get anyhow out of scope, and the .__del__() method would be called. This only if no other references are kept in the returned objects, which is also a condition for the current del network statement to work - so nothing would change from that point of view.
| operands = myconvertor.state_vector_operands() | ||
| operands = myconvertor.state_vector_operands() | ||
| else: | ||
| operands = None |
There was a problem hiding this comment.
What's the actual purpose of this?
If rank != 0, qibo_circ is fully ignored...
Even if it is somehow meaningful (I'm not seeing how, but that may be my limitation), the result could only be trivial, so you could even return immediately, without executing all the other operations...
There was a problem hiding this comment.
Each rank needs the same initial set of operands for computation. Here, the operands are created in Rank 0, for all other rank the operands are just set to None. In line 86, the operands created in Rank 0 is then broadcasted to all other ranks.
| @@ -62,6 +62,7 @@ def dense_vector_tn_MPI(qibo_circ, datatype, n_samples=8): | |||
| Dense vector of quantum circuit. | |||
| """ | |||
|
|
|||
| import cuquantum.cutensornet as cutn | |||
There was a problem hiding this comment.
Any reason to keep the imports within the functions? (instead of top-level)
I know it was like this even before this PR...
There was a problem hiding this comment.
The reason was that not all functions require the import, specifically dense_vector_tn(), expectation_pauli_tn(), dense_vector_mps(), pauli_string_gen(). Do you think it is better to bring them to the top-level?
| # Assign the device for each process. | ||
| device_id = rank % getDeviceCount() |
There was a problem hiding this comment.
Do you remember why it was repeated before?
There was a problem hiding this comment.
The comment may be still useful, and you could lift to the line above.
| @@ -136,6 +150,7 @@ def dense_vector_tn_nccl(qibo_circ, datatype, n_samples=8): | |||
| Returns: | |||
| Dense vector of quantum circuit. | |||
| """ | |||
| import cuquantum.cutensornet as cutn | |||
| @@ -200,6 +230,9 @@ def dense_vector_tn_nccl(qibo_circ, datatype, n_samples=8): | |||
| stream_ptr, | |||
| ) | |||
|
|
|||
| del network | |||
Hi,
This PR is to address issue #36.
Added memory management code and set the Pathfinding model used to CUTENSOR.