-
Notifications
You must be signed in to change notification settings - Fork 30
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 #213
Conversation
Sorry for not following up on this, are you still interested ? |
I am still interested, and if you also are, I'll go back and have a look at what I changed in the meantime (since I'm actively using those ideas locally for a package of mine) and update this request. |
Ok, thanks, I'll have a look at the update |
Now I updated this to my current state. And I also uploaded my current implementation of all this in DynamicPolynomials. |
src/complex.jl
Outdated
end | ||
return false | ||
end | ||
Base.conj(x::M) where {M<:AbstractMonomial} = |
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 add a shortcut here in case isreal(x)
is true
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 for the three methods below
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 the shortcuts; my initial rationale for not doing so was that the checks themselves are not just a query of a flag, but instead an iteration over potentially all the involved variables, so this is not a super cheap operation.
There's also the question of the function contract: Now it may return either the same object (for a no-op) or a new operation. Since MP is abstract, we don't know whether the underlying implementations are immutable/have addresses.
I added an isreal/iscomplex check for monomial vectors as well to have this explicitly and updated this in DynamicPolynomials, where it can be done much more efficiently.
Whew, when trying to implement the tests, I needed to go on the current master. I hope I found all the necessary replacements to the new conventions. Locally, all the tests work apart from two broken ones. One is due to |
@blegat , is there something that still prevents this from being merged? Everything should be adapted for the current version, the test are working by now with the DP implementation (though you can always argue that there could be more tests), the functions are documented... |
Thanks, no let's try to push this through the finish line. |
Co-authored-by: Benoît Legat <[email protected]>
Co-authored-by: Benoît Legat <[email protected]>
Co-authored-by: Benoît Legat <[email protected]>
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 for all the work, it's been a requested feature for a long time and the implementation looks neat. We can merge once ci passes
This is a first draft for the ideas mentioned in #212