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

Implementation of the K-support norm proximity operator #71

Merged
merged 14 commits into from
Jan 10, 2020

Conversation

LElgueddari
Copy link
Contributor

This implements the squared k-support norm and solves #68 .
The implementation of the proximity operator is based on this paper although it has originally been proposed here

@coveralls
Copy link

coveralls commented Nov 5, 2019

Pull Request Test Coverage Report for Build 216

  • 121 of 141 (85.82%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 93.766%

Changes Missing Coverage Covered Lines Changed/Added Lines %
modopt/opt/proximity.py 105 125 84.0%
Totals Coverage Status
Change from base Build 212: -0.6%
Covered Lines: 1850
Relevant Lines: 1973

💛 - Coveralls

@LElgueddari
Copy link
Contributor Author

Still Working on some tests to improve the coverage, but it would be cool to have a preliminary review

Copy link
Contributor Author

@LElgueddari LElgueddari left a comment

Choose a reason for hiding this comment

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

WIP PR: Still few things to change

modopt/opt/proximity.py Outdated Show resolved Hide resolved
modopt/opt/proximity.py Show resolved Hide resolved
modopt/opt/proximity.py Outdated Show resolved Hide resolved
modopt/opt/proximity.py Show resolved Hide resolved
self.ridge = proximity.Ridge(linear.Identity(), weights)
self.elasticnet_alpha_0 = proximity.ElasticNet(linear.Identity(),
alpha=0,
beta=weights)
self.elasticnet_beta_0 = proximity.ElasticNet(linear.Identity(),
alpha=weights,
beta=0)
self.one_support = proximity.KSupportNorm(beta=0.2, k_value=1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might need to change the beta parameter so it basically rescales wit the ridge in case of k_value = dimension

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to make this change in the current PR?

Copy link
Contributor Author

@LElgueddari LElgueddari Nov 19, 2019

Choose a reason for hiding this comment

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

Do you think it should be a great idea to have the beta parameter rescaled the same way, to have the same output as the ridge ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are the expert in this context so I have defer to your judgement. Either way I will be happy to merge the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm making it compatible with the ridge, it will ease the comparison between the two methods.

@LElgueddari
Copy link
Contributor Author

@sfarrens I'm not sure that I can add more tests to increase the coverage what do you think?

Comment on lines 700 to 702
\underset{y \in \mathbb{C}^{N}}{\text{min}} 0.5*\|x - y|\_2^2 +
\frac{\beta}{2}\text{min}\{\sum_{I \in \mathcal{G}_k}\|v_I\|_2^2:
\text{supp} (v_I) \subseteq I, \sum_{I \in \mathcal{G}_k} v_I = y\}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will render without the .. math:: command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expression is a bit complex, maybe we should just refer the user to the papers in references?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be completely acceptable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will refer to the paper

modopt/opt/proximity.py Outdated Show resolved Hide resolved
@sfarrens sfarrens merged commit 6215987 into CEA-COSMIC:master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants