feat: allow complex literals in defn #1572
Conversation
| # can be written in defn. | ||
| defp normalize({{:., dot_meta, [Complex, :new]}, meta, args}, state) do | ||
| {args, state} = normalize_list(args, state) | ||
| {{{:., dot_meta, [Complex, :new]}, meta, args}, state} |
There was a problem hiding this comment.
Should we use Complex.new or should we introduce Nx.c128 and friends exclusively for this? Especially because this will not work in practice:
defn foo(x) do
Complex.new(0, x)
end
So the usage in practice is quite limited?
There was a problem hiding this comment.
I generally use Complex.new instead of %Complex{re: ..., im: ...} to write a given complex literal, especially because Complex.new ensures that the components will be represented as floats. When structs are properly typed, this won't be an issue anymore.
We could just use %Complex{re: x, im: y} instead of Complex.new(x, y), but then we can end up with an invalid complex with integer (or worse, nil) components.
Nx.c64/1 and Nx.c128/1 would require a raw complex number to be given already, so it doesn't really solve the problem.
And we can't use Nx.complex because it will screw up the typing of the components and return a tensor instead of a constant.
There was a problem hiding this comment.
We can check that the arguments for Complex.new are valid (either numbers or non-finites) and raise otherwise, as this is specifically intended for literals and constants.
There was a problem hiding this comment.
If we add this check to Complex.new directly, we get a better error message, but as it is, it already fails to compile the defn, albeit with a "bad argument in arithmetic expression" error
There was a problem hiding this comment.
We could just use %Complex{re: x, im: y} instead of Complex.new(x, y), but then we can end up with an invalid complex with integer (or worse, nil) components.
To be clear, I am not proposing to use the structs. :D
Nx.c64/1 and Nx.c128/1 would require a raw complex number to be given already, so it doesn't really solve the problem.
I meant we could add a /2 version to them. My concern with this is that now everyone who consumes Nx.Defn.Expr has to deal with Complex as a new member of its AST, no? If we could formalize around tensor literals, it is less for downstream to handle.
There was a problem hiding this comment.
Complex.new produces a %Complex{} struct, which then becomes the value for a :constant Expr, which is already valid. This isn't too different from getting a complex value via a keyword list.
I don't see how adding this here changes the Nx.Defn.Expr.
There was a problem hiding this comment.
Ok, I have no further objections. :) ANd I assume Complex.new already checks the arguments anyway.
There was a problem hiding this comment.
I will update Complex with a better error message, but it will already fail as it is now
No description provided.