-
Notifications
You must be signed in to change notification settings - Fork 10
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
Mm/onnx parameter tweaks #443
Conversation
mmcleod89
commented
Nov 21, 2024
•
edited
Loading
edited
- Set the step size correctly for onnx model and standard l2-norm gradient
- Replace vague greek letters with descriptive names
…cs/sopt into mm/onnx_parameter_tweaks
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! 🚀 Just a couple of minor points to consider (see below).
cpp/sopt/differentiable_func.h
Outdated
@@ -34,6 +34,16 @@ template <typename SCALAR> class DifferentiableFunc | |||
// Calculate the function directly | |||
virtual Real function(t_Vector const &image, t_Vector const &y, t_LinearTransform const &Phi) = 0; | |||
|
|||
// Get appropriate gradient step-size for FISTA algorithms | |||
Real get_step_size() |
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.
Consider making this method const
?
cpp/sopt/onnx_differentiable_func.h
Outdated
SOPT_MEDIUM_LOG("Lipschitz Constant for CRR = {}", L_CRR); | ||
SOPT_MEDIUM_LOG("Step size for CRR = {}", this->step_size); | ||
} | ||
catch(std::exception e) |
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.
consider passing the exception by const ref?