-
Notifications
You must be signed in to change notification settings - Fork 329
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
BUG #394 fix prox and regularization #544
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #544 +/- ##
==========================================
+ Coverage 94.22% 94.23% +0.01%
==========================================
Files 37 37
Lines 4105 4045 -60
==========================================
- Hits 3868 3812 -56
+ Misses 237 233 -4 ☔ View full report in Codecov by Sentry. |
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.
Thanks for this work, Watcharin!
Within utils.base
- rename
lam
toregularizer_weight
orreg_weight_lambda
- Document shapes of inputs, add validation
- Move private functions inside of
get_...
functions - delete CAD
test_utils
- minimal number of elements for data and weights (2 instead of 3)
- Add a parameter for
reg_weight_lambda
to combine weighted and unweighted tests - Add assertion test for shapes of weight and shapes of data
- remove 2D tests
outside utils.base and test_utils
- restore
objective()
in stable_linear_sr3 - restore shape of thresholds in test_sr3_quadratic_library
- restore shape of thresholds in test_constrained_sr3_quadratic_library
- restore shape of thresholds in test_constrained_inequality_constraints
- move to private function - rename lam to regularization_weight - remove cad - add docstring - add validation
I double-checked StableLinearSR3 objective function and it is the same as SR3. It also seems that the examples in the notebook use regularization_weight of the same shape as data input but the ones in the unit test use the transposed version. So there are two conflicting examples. Which one should we choose to be correct? |
It seems like the example notebooks call SR3, which then transposes before calling these functions. That said, for these helper functions, the data should be in the same shape as the weights. |
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.
Good work Watcharin! Most of what remains is cleaning this up and cutting out stuff that can be made redundant.
@Jacob-Stevens-Haas |
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.
Good work, Watcharin! We're in the home stretch. Mostly just deletion and edits
@Jacob-Stevens-Haas I ran |
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.
Well done, Watcharin! Looks like you fixed the notebook issue.
#394
In the A unified sparse optimization framework to learn parsimonious physics-informed models from data paper, in each iteration, prox is called with
lambda * nu
, so if we setlambda = (threshold^2)/(2*nu)
then for l0, coefficients smaller than the threshold will be eliminated.This pull request only fixes the issue for prox and regularization calculation but it is still being called incorrectly in SR3 class (except for l0). Future PR will be needed to accept weight lambda correctly.