-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Atom for the Von Neumann Entropy #1789
Conversation
get vn_entr canonicalization *probably* working
"it's respective Canonicalization method" should be "its respective Canonicalization method". |
The failing tests seem to be for versions of Python where SCS 2.1.2 is installed, rather than SCS 3. I ran the tests on my laptop with SCS 3 and they passed, but the return code was "inaccurate" even after I increased |
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.
@aryamanJgl here are some things to be aware of. I'm investigating other stuff and hopefully will provide more comments soon.
cvxpy/tests/test_vn_entr.py
Outdated
expect_N = np.array([[0.36787973, 0.00099987], | ||
[0.00099987, 0.36787973]]) | ||
eps = 1e2 | ||
cons1 = N >> 0 |
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.
The constraint that N >> 0
is redundant, since you already declared PSD = True
for N
. Same comment goes for other tests.
cvxpy/tests/test_vn_entr.py
Outdated
N = U.T@N@U | ||
objective = cp.Maximize(vn_entr(N)) | ||
obj_pair = (objective, 1.1033501770078251) | ||
con_pairs = [ | ||
(cons1, None), | ||
(cons2, None), | ||
(cons3, None) | ||
] | ||
var_pairs = [ | ||
(N, expect_N) |
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.
Here you're reassigning N
. That should cause tests to fail since expect_N
is supposed to be with respect to the original variable.
cvxpy/tests/test_vn_entr.py
Outdated
eps = 1e3 | ||
cons1 = N >> 0 | ||
cons2 = N << eps*np.eye(N.shape[0]) | ||
cons3 = N[2][0] == 1e-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.
This is inconsistent with expect_N
.
@aryamanJgl here is a tiny example of a test that's failing:
The solver (SCS or MOSEK) says the problem is infeasible, but Here's another option:
In fact, you can make that example even simpler by setting |
Progress toward working vn_entr canonicalization
Riley Murray seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Isn't this a another case where a long name is needed? vn_entr is completely cryptic. I would go for von_neumann_entropy. |
That's a good point. Although I'd vote for @aryamanJgl if you don't object, how about you change it to |
Yeah, that makes a lot of sense! I will change it in the next commit. |
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.
Mostly looks good! The main thing that stands out is the need for documentation. The docstring for von_neumann_entr
is missing, and the web docs need to be updated to mention this new atom. I also left minor comments on the tests.
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.
Looks good to me!
@SteveDiamond can you review this PR? We need to project maintainers to approve before merging, since this affects the public API. |
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.
Approved subject to my comments. Also shouldn't we add it to the table of atoms?
@aryamanJgl please address my response to Steven's comment. Also, there are two things to do for the docs. |
* origin/master: Add autoflake pre-commit; remove unused variable; remove unused pass (#1756) [New atom] dotsort (#1803) Change exception type when unsupported SCIP 4.x is installed (#1807) Atom for the Von Neumann Entropy (#1789) Exclude examples from deployment (#1804) make upload jobs pass (#1802) [CI] Replace manylinux_2_24 image (#1792)
In this PR I add support for the Von Neumann Entropy as an
Atom
. Thevn_entr
class itself lies incvxpy/atoms
, and its respective Canonicalization method lies incvxpy/reductions/dcp2cone/atom_canonicalizers
, finally, the tests reside incvxpy/tests/test_vn_entr.py
. I chose to omit the_grad
method for this PR as per @SteveDiamond's advice.The implementation of the function itself is wrapped in the$S(\rho)=-\sum_x \lambda_x\log\lambda_x$ --- this form can be found in equation (11.40) of Neilsen & Chuang's QIC text.
numeric
procedure invn_entr.py
---The Canonicalization procedure is based off of Proposition-4 of THIS paper.