Skip to content

Clean up g_proximal arguments #321

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 9, 2023
Merged

Clean up g_proximal arguments #321

merged 7 commits into from
Jan 9, 2023

Conversation

tkoskela
Copy link
Contributor

@tkoskela tkoskela commented Dec 8, 2022

Closes actions/setup-python#298

Pass beta and phi through constructor of g_proximal and pass them to arguments of l1_proximal internally when the proximal function is called. Remove them from call in forward_backward in preparation for other (TF) g_proximal implementations.

@tkoskela
Pass beta and phi through constructor of g_proximal.
e9e8b8b 
Remove them from call in forward_backward in preparation for
other (TF) g_proximal implementations.
@tkoskela
Copy link
Contributor Author

tkoskela commented Dec 8, 2022

We fail test_forward_backward because I've changed the call signature of g_proximal. In the test, g_proximal is proximal::id:

sopt/cpp/sopt/proximal.h

Lines 136 to 139 in d45d533

void id(Eigen::DenseBase<T0> &out, typename real_type<typename T0::Scalar>::type gamma,
Eigen::DenseBase<T1> const &x) {
out = x;
}

so the difference here is what we pass in as x.

@tkoskela
Copy link
Contributor Author

I would like to pass beta and Phi through setters of the l1_proximal rather than the constructor.

@tkoskela
Copy link
Contributor Author

tkoskela commented Jan 5, 2023 •
edited
Loading

Nothing says happy 2023 like all MacOS tests failing for no reason! It seems we are affected by actions/runner-images#11545 looks like people are actively working to fix it so maybe it'll be resolved soon 🤞

@tkoskela tkoskela requested a review from SJaffa January 5, 2023 12:06
SJaffa
SJaffa reviewed Jan 6, 2023
View reviewed changes
SJaffa
SJaffa requested changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@SJaffa SJaffa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks good, I just suggested some tidying and comments.

The MacOS g++ tests are failing with error:
ConanException: sha256 signature failed for 'jpegsrc.v9e.tar.gz' file.

Is that to do with the homebrew issue you mentioned?

@tkoskela
Copy link
Contributor Author

tkoskela commented Jan 6, 2023

The MacOS g++ tests are failing with error:
ConanException: sha256 signature failed for 'jpegsrc.v9e.tar.gz' file.

I managed to work around the homebrew issue by skipping brew update, then ran into that and made a note of it in #327.

It looks like it might have been updated 12 hours ago conan-io/conan-center-index#15093

@tkoskela tkoskela merged commit 66add11 into development Jan 9, 2023
@tkoskela tkoskela deleted the tk/g_proximal_args branch January 9, 2023 16:04
@tkoskela tkoskela mentioned this pull request Feb 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SJaffa SJaffa SJaffa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Built-in caching for poetry dependencies
2 participants
@tkoskela @SJaffa