-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support using complex numbers and lists of complex numbers inside kernel functions #1518
Support using complex numbers and lists of complex numbers inside kernel functions #1518
Conversation
CLA Assistant Lite bot All Contributors have signed the CLA. |
I have read the Contributor License Agreement and I hereby accept the Terms. |
recheck |
python/cudaq/kernel/ast_bridge.py
Outdated
""" | ||
ty = self.getFloatType() | ||
return arith.ConstantOp(ty, self.getFloatAttr(ty, value)).result | ||
|
||
def getComplexType(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to parameterize this with the precision or bitwidth in MLIR terms (32 or 64 is what we support)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
python/cudaq/kernel/ast_bridge.py
Outdated
""" | ||
return ComplexType.get(self.getFloatType()) | ||
|
||
def getConstantComplex(self, value): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (parameterize with 32 or 64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
python/cudaq/kernel/ast_bridge.py
Outdated
@@ -2726,6 +2730,22 @@ def visit_Continue(self, node): | |||
else: | |||
cc.ContinueOp([]) | |||
|
|||
def promote_operand_type(self, ty, operand): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this up with the other utility methods on this visitor class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -2896,16 +2906,21 @@ def visit_Name(self, node): | |||
|
|||
if node.id in self.capturedVars: | |||
# Only support a small subset of types here | |||
complexType = type(1j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just use complex
instead of getting the type of 1j
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also probably check for np.complex64
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just use complex instead of getting the type of 1j
We use complex dialect and it seems to override the complex type. Maybe worth decoupling in a separate PR...
We should also probably check for np.complex64
Done.
def test_float_vec_param(vec : list[float]): | ||
f1 = vec | ||
|
||
counts = cudaq.sample(test_float_vec_param, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better test here would be to define a complex
return type on the kernel, and assert test_float_vec_param(f) == that value
something like
@cudaq.kernel
def test_float_vec_param(vec : list[float], i : int) -> complex:
f1 = vec
return vec[i]
for i in range(len(f)):
assert np.isclose(f[i], test_float_vec_param(f, i), atol=1e-6)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opps, looks like there is a bug there - function above always returns the value of f[0]
. Will investigate...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the tests for now to work around this issue (will update the tests after the issue is fixed in the experimental branch as well)
counts = cudaq.sample(test_float_np_use) | ||
assert len(counts) == 0 | ||
|
||
def test_complex(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest also adding support for c = np.array([...], dtype=np.complex64)
and demonstrate with the nvidia
target. Also it would be good to test that we raise an exception for the use of complex
with nvidia
and np.complex64
with FP64 backends (all the other ones).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a separate issue for supporting np arrays: #1523, will work in it in the next PR
python/cudaq/kernel/ast_bridge.py
Outdated
""" | ||
Return an MLIR complex type (double precision). | ||
""" | ||
return ComplexType.get(self.getFloatType()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is there a way to request either F32 or F64 in the python bindings? (This getFloatType() feels more ambiguous.)
python/cudaq/kernel/ast_bridge.py
Outdated
|
||
if F64Type.isinstance(ty): | ||
if IntegerType.isinstance(operand.type): | ||
operand = arith.SIToFPOp(ty, operand).result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These conversions make me wonder if we don't want to automatically add FPTrunc and FPExt if the input data is not at the precision expected by the simulator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to postpone this to the next PR(s) when I have examples that are running with different data types in np.array
and assigning the state to a qvector
(to be able to tests my changes properly) - does it is sound reasonable?
Replaced by #1550 |
Make sure we can use complex numbers inside kernel function definitions, for example:
Towards: #1086
Closes: #1454