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

calculate diamond norm using dual problem #54

Merged
merged 4 commits into from
Mar 15, 2019
Merged

Conversation

benkj
Copy link
Contributor

@benkj benkj commented Mar 13, 2019

This implements the same algorithm of QETLAB, first discussed in this paper, section 3

Fix #53

@pgawron pgawron requested review from lpawela and pgawron and removed request for lpawela March 13, 2019 23:31
Copy link
Collaborator

@pgawron pgawron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for diamond distance symmetry should be added to the test/convex.jl

@lpawela lpawela self-requested a review March 13, 2019 23:56
Copy link
Member

@lpawela lpawela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be simply Z in :SDP instead of Z+Z' in :SDP? Except fixing a bug, which I cannot reproduce, does the dual problem have advantages over the primal one? Better numerical performance?

@benkj
Copy link
Contributor Author

benkj commented Mar 14, 2019

Shouldn't it be simply Z in :SDP instead of Z+Z' in :SDP? Except fixing a bug, which I cannot reproduce, does the dual problem have advantages over the primal one? Better numerical performance?

I took inspiration from qetlab where they have the similar line

       cons = [Y0,-Phi;-Phi',Y1];
        cons + cons' >= 0; % avoid some numerical problems: CVX often thinks things aren't symmetric without this

I also did notice some instability with just Z in :SDP.

I was just playing with primal & dual formulation, and found that the dual one didn't have the buggy behaviour of #53 . Not sure if it has other advantages.

Copy link
Member

@lpawela lpawela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep both versions, norm_diamond should take an optional parameter likedual=false setting which approach is used.

@benkj
Copy link
Contributor Author

benkj commented Mar 14, 2019

I agree that we should keep both versions. We should also force the correct eps. Why did you choose eps=1e-7?

@lpawela
Copy link
Member

lpawela commented Mar 14, 2019

After some tests on this problem anything higher causes to hit max_iters limit. Also, I recall running into problems in other optimizations when eps was set to something smaller.

@benkj
Copy link
Contributor Author

benkj commented Mar 14, 2019

Ok. Does it make sense then do add eps as a parameter?
In case we can hit the max_iter with larger dimensional problems.

@lpawela
Copy link
Member

lpawela commented Mar 14, 2019

Yes, the user should be able to set eps and max_iters.

@benkj
Copy link
Contributor Author

benkj commented Mar 14, 2019

In the new commit I added a test from #53 and implemented both the primal and dual problem with optional argument

@pgawron
Copy link
Collaborator

pgawron commented Mar 14, 2019

I think we should keep both versions, norm_diamond should take an optional parameter likedual=false setting which approach is used.

We should have method=:primal as default with option for method=:dual. I believe it would be more Julianesque and elegant.

@benkj
Copy link
Contributor Author

benkj commented Mar 15, 2019

I have implemented the 'julianeque' notation with method=:primal as default choice

@pgawron pgawron merged commit f0eb3d4 into iitis:master Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants