Skip to content

Conversation

@arnavk23
Copy link

@arnavk23 arnavk23 commented Oct 1, 2025

Successfully implemented the generalization from Binf (L∞-ball) constraints to Box constraints across all applicable shifted proximal operators, following the pattern established in PR #104 for ShiftedRootNormLhalfBox.

Key Changes

  1. Enhanced existing Box variants: ShiftedIndBallL0Box, ShiftedGroupNormL2Box, and ShiftedRootNormLhalfBox
    Dual signature support: Each Box variant now supports both:
    General box constraints: shifted(h, x, l, u) for arbitrary bounds [l, u]
    Symmetric constraints: shifted(h, x, Δ, χ) for symmetric bounds [-Δ, Δ] (backward compatibility)

  2. Box implementations now override the corresponding Binf method signatures
    Legacy code using (Δ, χ) signatures automatically returns Box variants instead of Binf variants
    Maintains full backward compatibility while providing enhanced functionality

  3. Enhanced getproperty method to provide backward-compatible access to ψ.Δ and ψ.χ properties on Box variants
    Automatic conversion from symmetric box constraints [-Δ, Δ] back to radius Δ when accessed
    Seamless integration with existing code expecting Binf-style properties

  4. Updated all test references from ShiftedGroupNormL2Binf and ShiftedIndBallL0BInf to their respective Box variants
    Fixed type expectations to match Box variant signatures with additional type parameters
    Implemented appropriate numerical tolerance using isapprox with atol for precision differences between algorithms

  5. Verified set_radius! methods work correctly with all Box variants
    Proper export statements and module includes already in place
    Smart bounds conversion: set_radius!(ψ, Δ) automatically sets bounds to [-Δ, Δ]

Closes #88

@arnavk23
Copy link
Author

arnavk23 commented Oct 1, 2025

@dpo please review this pr.

@dpo
Copy link
Member

dpo commented Oct 1, 2025

Many thanks @arnavk23 ! I'll have a look. Please don't commit the Manifest.toml though.

@dpo dpo requested a review from Copilot October 1, 2025 14:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR successfully generalizes shifted proximal operators from Binf (L∞-ball) constraints to Box constraints, enabling support for arbitrary box bounds [l, u] while maintaining backward compatibility with symmetric constraints [-Δ, Δ].

Key Changes

  • Enhanced existing Box variants (ShiftedIndBallL0Box, ShiftedGroupNormL2Box) with dual signature support for both general and symmetric constraints
  • Added backward compatibility layer that automatically returns Box variants for legacy Binf signatures
  • Updated test references and type expectations to match the new Box variant signatures

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
test/runtests.jl Updated test references from Binf to Box variants and adjusted type expectations with additional type parameters
src/shiftedIndBallL0Box.jl New Box variant for IndBallL0 with general box constraints and backward compatibility
src/shiftedGroupNormL2Box.jl New Box variant for GroupNormL2 with general box constraints and backward compatibility
src/ShiftedProximalOperators.jl Added includes for new Box variants and enhanced getproperty method for backward compatibility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 72.34043% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.54%. Comparing base (753a97f) to head (aa30ee7).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
src/shiftedIndBallL0Box.jl 57.57% 14 Missing ⚠️
src/shiftedGroupNormL2Box.jl 80.00% 9 Missing ⚠️
src/ShiftedProximalOperators.jl 81.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   73.71%   72.54%   -1.18%     
==========================================
  Files          22       26       +4     
  Lines         898     1060     +162     
==========================================
+ Hits          662      769     +107     
- Misses        236      291      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arnavk23 arnavk23 requested a review from Copilot October 11, 2025 08:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@dpo
Copy link
Member

dpo commented Oct 11, 2025

@arnavk23 I think one of the warnings due to methods being overwritten is caused by this PR?! I'll open issues for the others.

-    @test @wrappedallocs(prox!(y, ϕ, x, ν)) == 0
+    @test @wrappedallocs(prox!(y, ϕ, x, ν)) <= 8
        @test @wrappedallocs(prox!(y, ϕ, x, ν, dims=1)) <= 8
        @test @wrappedallocs(prox!(y, ϕ, x, ν, dims=2)) <= 8
    end
@arnavk23 arnavk23 requested a review from Copilot October 12, 2025 19:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@arnavk23 arnavk23 requested a review from Copilot October 15, 2025 20:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

arnavk23 and others added 2 commits October 16, 2025 02:37
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
arnavk23 and others added 7 commits October 16, 2025 02:38
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…ocations in group prox implementations; update includes and tests (follow PR JuliaSmoothOptimizers#104 pattern)
@arnavk23 arnavk23 requested a review from Copilot October 17, 2025 18:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Copilot <[email protected]>
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.

Generalize Binf → Box

2 participants