-
Notifications
You must be signed in to change notification settings - Fork 26
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
WIP: fixing broken support for complex numbers and adding new complex functionality #13
base: master
Are you sure you want to change the base?
Conversation
I am currently working on making the compressed sensing forward-backward example from the tutorial work. So far the example seems able to complete without discarding imaginary parts anywhere, but at least the l-2 norm still computes wrong and my so far un-committed attempts at fixing it still lead to wrong solutions... |
The above-mentioned compressed sensing forward-backward example now seems to work correctly. |
@@ -502,8 +502,9 @@ def _prox(self, x, T): | |||
sol = x + 2. * gamma * self.At(self.y() * self.w**2) | |||
sol /= 1. + 2. * gamma * self.nu * self.w**2 | |||
else: | |||
res = minimize(fun=lambda z: 0.5 * np.sum((z - x)**2) + gamma * | |||
np.sum((self.w * (self.A(z) - self.y()))**2), | |||
res = minimize(fun=lambda z: 0.5 * np.sum(np.abs(z - x)**2) |
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 might be worth replacing the sum-abs operations with inner products with the complex conjugate of vectors here. Pro: abs
makes these operations quite a bit slower. Con: I think the sum-abs formulation is a bit more readable, see line 496.
This is a proposed implementation that does away with the `handle_complex` option and determines the method automatically. I also added tests to validate the function with complex input.
This should initialize the `sol` variable of the forward-backward solver as a complex variable if necessary. This is determined from whether y, A*x or AT*y of any of the objective function constituents is complex.
The squared two-norm could not handle complex vectors correctly (absolute value was omitted).
I am adding tests to test everything with complex numbers as well. Should I test with complex numbers in addition to the already existing tests, or should I replace the numbers in the existing tests with complex numbers? In principle, testing everything with complex numbers should be more general and ensure that it works with real numbers as well, but maybe someone would like to keep the strictly real tests? |
I have been thinking about this adaptation of the toolbox to handle complex numbers. Wouldn't it be better to just input n-by-2 matrices instead of n-dimensional complex vectors? Are there any operations currently implemented in pyunlocbox that would fail if you were simply to add an extra dimension to your input to account for the imaginary part? |
Hi @ThomasA. I know it's been a long time, but may I ask what is the status of this PR? (There are some more commits in another branch of yours at https://github.com/ThomasA/pyunlocbox/commits/fix_complex_tmp.) |
@ThomasA if you want to finish this, I would be happy to help. |
It has been so long now that I need to dig around a bit to remember where I got to, but it would actually be nice exercise to finish this up. |
This is a work-in-progress pull request that I recommend not to merge in until I am done fixing and implementing new complex support features. I will be adding commits to this branch as I go along.
This PR addresses #11