Skip to content
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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ThomasA
Copy link
Contributor

@ThomasA ThomasA commented Mar 29, 2018

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

@coveralls
Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage decreased (-0.2%) to 98.376% when pulling a9cfec3 on ThomasA:fix_complex into ee37c50 on epfl-lts2:master.

@ThomasA
Copy link
Contributor Author

ThomasA commented Apr 1, 2018

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...

@ThomasA
Copy link
Contributor Author

ThomasA commented Apr 1, 2018

The above-mentioned compressed sensing forward-backward example now seems to work correctly.
The next steps will be to determine which other parts of the package may have complex number issues and also to implement further tests to validate the so far altered code for complex numbers.

@@ -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)
Copy link
Contributor Author

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.

Thomas Arildsen added 4 commits July 5, 2018 11:22
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.
Thomas Arildsen added 2 commits July 5, 2018 18:41
The squared two-norm could not handle complex vectors correctly
(absolute value was omitted).
@ThomasA
Copy link
Contributor Author

ThomasA commented Jul 6, 2018

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?

@rodrigo-pena
Copy link
Collaborator

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?

@mdeff
Copy link
Collaborator

mdeff commented Mar 24, 2020

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.)

@mdeff mdeff marked this pull request as draft April 8, 2020 21:53
@nperraud
Copy link
Collaborator

@ThomasA if you want to finish this, I would be happy to help.

@ThomasA
Copy link
Contributor Author

ThomasA commented Jan 27, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants