-
Notifications
You must be signed in to change notification settings - Fork 21
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
Complex-valued variables #121
Conversation
Issue with sometimes working implementation for incomplete substitution (I just quote the code comment)
Design question: With the new implementation, the "dynamic" part becomes slightly more "typed", as the variable and monomial ordering are now also part of the type. Still, the complex kind is a field instead of a type parameter. While I definitely would keep this to distinguish between the variable, its conjugate, its real and imaginary part (else all containers holding such variables would need to be abstract containers, making this very inefficient), it may be debatable whether the issue of "is this a complex type at all" should be encoded as a type parameter. Pro: Some replacements may be simpler to decide based on static dispatch. Con: Duplication of information; and mixing real and complex variables again has to fall back to a generic implementation. |
Yes, I would definitely not incorporate whether it's a real, imaginary part etc... as type as it would disallow mixing different ones in a polynomial etc... |
This would depend on the actual implementation in |
@projekter I forgot about this PR, we should merge it. I'll try to take a look next week, remind me if I don't |
@blegat Here's a reminder |
src/subs.jl
Outdated
# imaginary part is incomplete), this is an impossible conversion. | ||
# - The coefficients in vals might not be complex-valued; but to assign only a part of the variable, we necessarily need to | ||
# introduce an explicit imaginary coefficient to the value. | ||
# Currently, we don't do anything to catch these errors. |
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 rather have a solid implementation that only handle some cases ans error in other cases than an implementation that does not error but that may have incorrect behavior.
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.
Ok, now I modified it to a different implementation. Basically, real and imaginary parts, when occurring in the substitution rules, are seen as assignments to real values, so they won't act on the complex variables at all, if those occur. You could then say that this (call it lexicographic substitution) is inconsistent with the complex variable in the rule still acting on the real and imaginary parts, and I agree. However, where to draw the line? It is then also inconsistent to apply it to the conjugate, but this is (I think) a very reasonable choice.
I guess that a typical situation will probably never mix real/imaginary part variables with full complex variables, so I don't expect whatever behavior is defined here to actually be used somewhere.
src/subs.jl
Outdated
vals[j] = value | ||
elseif vars[j].kind != REAL_PART | ||
error( | ||
"Found complex variable with substitution of imaginary part - not implemented", |
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.
Nice, could you add some tests for these edge cases ?
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 added a few testcases. Note that I did not use the substitution syntax without explicit variable specification. This is because variables(p)
would list all variables, complex, conjugates, real, and imaginary parts separately. So you'd have to specify values for all of them (unnecessarily - and this would even be more restrictive as then: z + real(z)
, called with arguments (1+2im, 1)
would trigger the new error, because z
occurs but a value is assigned to real(z)
). Either the whole variable assignment would need to be redesigned or we just say that for complex variables, you should always explicitly specify a substitution rule.
(But even then, substitution rules could be redundant; [z, real(z)] => [1+2im, 1]
is also a valid rule. Should we check whether the rules are self-consistent? Given that variables occuring twice are also not checked, I think the current lax behavior is fine.
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.
Indeed, that's an issue. We should only allow COMPLEX_PART
to used in substitutions then. In this PR, I think we should target the simplest implementation that's bullet proof. We can try to make something more complicated but I'd rather do it in a separate PR if there is a use case for it. So I'd just restrict z
to be a COMPLEX_PART
whenever z => ...
is used
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.
This simplifies the substitution process significantly. I don't know whether I like it, but there's some sense to it.
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 can revisit in a follow up PR without being breaking. We'll be able to recover your implementation easily from this branch.
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.
Thanks a lot for pushing this! It's been a big request from many users. I'll merge once CI turns green
- Macro for complex variables is now called `@complex_polyvar` - DP does not support partial substitutions, so disable the tests
* Update for changes discussed in JuliaAlgebra/DynamicPolynomials.jl#121 - Macro for complex variables is now called `@complex_polyvar` - DP does not support partial substitutions, so disable the tests * Add explanation, format
This is in conjuction with JuliaAlgebra/MultivariatePolynomials.jl#213 and provides the DynamicPolynomials implementation of the complex-valued variable functionality.