Skip to content

Add PrecomposedSlicedSeparableSum implementation and corresponding tests#157

Merged
lostella merged 2 commits into
JuliaFirstOrder:masterfrom
hakkelt:precomposed-sliced-separable-sum
May 19, 2026
Merged

Add PrecomposedSlicedSeparableSum implementation and corresponding tests#157
lostella merged 2 commits into
JuliaFirstOrder:masterfrom
hakkelt:precomposed-sliced-separable-sum

Conversation

@hakkelt
Copy link
Copy Markdown
Contributor

@hakkelt hakkelt commented Apr 13, 2026

This new function implements the intersection/composition of Precompose and SlicedSeparableSum. The long name suggests a code smell, but I could not find a cleaner way to implement this composition, so I'm open to better solutions. The goal is to implement prox for tricky expressions like the following:

x = (randn(10), randn(10))
norm(x[1], 1) + norm(A2[1:5, 1:5] * x[2][1:5], 2) + norm(A2[6:10, 6:10] * x[2][6:10], 2)^2

@hakkelt hakkelt marked this pull request as ready for review April 13, 2026 17:56
@hakkelt hakkelt marked this pull request as draft April 13, 2026 18:12
@hakkelt hakkelt force-pushed the precomposed-sliced-separable-sum branch from d947f82 to da2eaa3 Compare April 13, 2026 18:29
@hakkelt hakkelt marked this pull request as ready for review April 13, 2026 18:53
@hakkelt
Copy link
Copy Markdown
Contributor Author

hakkelt commented Apr 13, 2026

@lostella: Could you review this PR?

The benchmarking is still failing because of the lack of permission. 😞 My guess is that adding write permisison to workflows (Settings -> Actions -> General -> Workflow permissions -> Read repository contents and packages permissions) might solve the problem. I'm not really expert on security, so I'm not sure how dangerous it is exactly. What I read about that is that it can lead to exposure of secrects (e.g. if someone creates a PR that modifies the benchmarking script such that it sends the GITHUB_TOKEN to an external server). But as your approval is required for PRs of new contributors by the current settings, the threat is greatly reduced, I think.

The other option could be to disable commenting and push the results only to job summary (https://astroautomata.com/AirspeedVelocity.jl/stable/#Option-2:-Job-Summary).

@lostella
Copy link
Copy Markdown
Member

@hakkelt will review asap.

I agree that probably disabling comments from the benchmarking workflow, and logging the result instead, is the safest at the moment. Does the workflow fail in case of performance regressions? If so, that should be enough: red => inspect logs

@lostella lostella self-requested a review April 13, 2026 19:37
@hakkelt
Copy link
Copy Markdown
Contributor Author

hakkelt commented Apr 19, 2026

@lostella I'll fix the benchmarking problem in a separate PR.

@hakkelt hakkelt force-pushed the precomposed-sliced-separable-sum branch from da2eaa3 to 91ca992 Compare April 21, 2026 11:14
@hakkelt
Copy link
Copy Markdown
Contributor Author

hakkelt commented May 19, 2026

Is there anything I can do for this PR or help the reviewing process?
Btw, the benchmarking results are available here, and they seem to be ok (which is expected as this PR only adds features but doesn't modify any existing functions): https://github.com/JuliaFirstOrder/ProximalOperators.jl/actions/runs/24719301336

Copy link
Copy Markdown
Member

@lostella lostella left a comment

Choose a reason for hiding this comment

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

@hakkelt looks good! Sorry, fell off my radar!

@lostella lostella merged commit 57165e7 into JuliaFirstOrder:master May 19, 2026
8 checks passed
@hakkelt
Copy link
Copy Markdown
Contributor Author

hakkelt commented May 19, 2026

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants