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.
| 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 | |||
src/qibotn/eval.py
Outdated
| @@ -200,6 +230,9 @@ def dense_vector_tn_nccl(qibo_circ, datatype, n_samples=8): | |||
| stream_ptr, | |||
| ) | |||
|
|
|||
| del network | |||
|
@alecandido @Tankya2 what is the plan for this PR? |
Hi @scarrazza, @alecandido , @liweintu , I believe most issues in this PR have been addressed. The remaining concern is the presence of duplicated code across various computation modes. I propose to close this PR first if there are no other further issues and to tackle these duplicated codes in another PR. |
|
I tested this PR branch on ASPIRE 2A+, using # Test Cuquantum
cutn_time, result_tn = time(
lambda: qibotn.eval.dense_vector_tn(qibo_circ, dtype).flatten()
)
> assert 1e-2 * qibo_time < cutn_time < 1e2 * qibo_time
E assert 0.38179754093289375 < (100.0 * 0.00212192814797163)Seems the time taken is too long to pass the assertion? @Tankya2 |
|
My main concern was that the code was extended, introducing even further duplication, without reducing the existing one. Tests and performance benchmarks are not automated, so little can be said. However, if you manually test the code, and this is needed soon, of course feel free to proceed. Just take note of the project needs, and schedule them for later. |
Hi,
This PR is to address issue #36.
Added memory management code and set the Pathfinding model used to CUTENSOR.