Migration to QuEST v4 API#17
Conversation
rrmeister
left a comment
rrmeister
left a comment
There was a problem hiding this comment.
Thanks very much for the PR, Maurice! Looks very promising for porting pyQuEST to QuESTv4. I've had a skim through the changes and have left some comments. Some are quite nit-picky, but others are rather critical. Have a look and see if they all make sense.
Cheers!
Richard
pyquest/core.pyx
Outdated
| quest.seedQuEST(&(self.c_env), seeds, num_seeds) | ||
| free(seeds) | ||
| def seeds(self, value): | ||
| cdef unsigned[:] seed_array = np.asarray(value, dtype=np.uint32) |
There was a problem hiding this comment.
Casting to an unsigned type without checking for negative values will underflow and give a numpy warning. Probably better to explicitly check and throw an appropriate error when trying to seed with a negative value.
pyquest/core.pyx
Outdated
| free(seeds) | ||
| def seeds(self, value): | ||
| cdef unsigned[:] seed_array = np.asarray(value, dtype=np.uint32) | ||
| if seed_array.size != quest.getNumSeeds(): |
There was a problem hiding this comment.
The seed array for QuEST doesn't need to remain constant in size.
pyquest/core.pyx
Outdated
|
|
||
| quest.setQuregToWeightedSum(res_reg.c_register, &(coeffs[0]), &(quregs[0]), 2) | ||
|
|
||
| # Result has scaling factor 1 since it's the difference |
There was a problem hiding this comment.
Since res_reg is a new Register, its scaling factor is already 1, no need to set this expliticly.
pyquest/core.pyx
Outdated
|
|
||
| quest.setQuregToWeightedSum(res_reg.c_register, &(coeffs[0]), &(quregs[0]), 2) | ||
|
|
||
| # Result has scaling factor 1 since it's the sum |
pyquest/core.pyx
Outdated
| # Create result = 1.0 * left_scaling * left - 1.0 * right_scaling * right | ||
| coeffs[0] = left_reg._scaling_factor | ||
| coeffs[1].real = -right_reg._scaling_factor.real | ||
| coeffs[1].imag = -right_reg._scaling_factor.imag |
There was a problem hiding this comment.
Does this need the real and imag parts set separately? I would have thought
coeffs[1] = -right_reg._scaling_factor
would suffice.
| for k in range(matrix_dim): | ||
| self._real[k] = (<ComplexMatrix2*>self._matrix).real[k] | ||
| self._imag[k] = (<ComplexMatrix2*>self._matrix).imag[k] | ||
| self._matrix = malloc(sizeof(quest.CompMatr1)) |
There was a problem hiding this comment.
This does not initialise all fields of the CompMatr1 struct and will cause an error when applied to a Register.
pyquest/operators.pyx
Outdated
| for m in range(arr.shape[1]): | ||
| self._real[k][m] = arr[k, m].real | ||
| self._imag[k][m] = arr[k, m].imag | ||
| self._set_matrix_element(k, m, <qcomp>(arr[k, m].real + 1j * arr[k, m].imag)) |
There was a problem hiding this comment.
Worth checking if manually assembling a complex number here is really necessary or if the cast to qcomp would take care of that. Also in all following functions.
pyquest/operators.pyx
Outdated
| quest.initDiagonalOp(self._diag_op, &real_el[0], &imag_el[0]) | ||
| # Convert Python complex array to qcomp array | ||
| num_elems = diag_elements.size | ||
| qcomp_el = np.ascontiguousarray(diag_elements, dtype=np.complex128) |
There was a problem hiding this comment.
This should probably use the data type QuEST uses, not just always complex128.
| for k in range(matrix_dim): | ||
| self._real[k] = &(<ComplexMatrix2*>self._matrix).real[k][0] | ||
| self._imag[k] = &(<ComplexMatrix2*>self._matrix).imag[k][0] | ||
| self._matrix = malloc(sizeof(quest.CompMatr1)) |
There was a problem hiding this comment.
Does not properly init all fields of CompMatr1.
| for k in range(matrix_dim): | ||
| self._real[k] = &(<ComplexMatrix4*>self._matrix).real[k][0] | ||
| self._imag[k] = &(<ComplexMatrix4*>self._matrix).imag[k][0] | ||
| self._matrix = malloc(sizeof(quest.CompMatr2)) |
There was a problem hiding this comment.
Does not properly init all fields of CompMatr2.
3 participants
Updates to migrate pyQuEST to QuEST v4