Skip to content

simplify objective functions#15

Merged
timholy merged 4 commits intoHolyLab:mainfrom
oscardssmith:patch-1
Mar 25, 2026
Merged

simplify objective functions#15
timholy merged 4 commits intoHolyLab:mainfrom
oscardssmith:patch-1

Conversation

@oscardssmith
Copy link
Copy Markdown
Contributor

since the log(A_ij) is constant, there's no need to include it in the linear objectives, which allows for simplifying the loop dramatically (at least when the matrix has no zeros)

since the log(A_ij) is constant, there's no need to include it in the linear objectives, which allows for simplifying the loop dramatically (at least when the matrix has no zeros)
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.87%. Comparing base (ce4a3f2) to head (a8cf11a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   92.84%   92.87%   +0.03%     
==========================================
  Files           5        5              
  Lines         852      856       +4     
==========================================
+ Hits          791      795       +4     
  Misses         61       61              

☔ 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.

model = JuMP.Model(HiGHS.Optimizer)
JuMP.set_silent(model)
@variable(model, α[1:n])
@objective(model, Min, sum(α[i] + α[j] - logA[i, j] for i in 1:n, j in 1:n if A[i, j] != 0))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're right this is a good simplification (although it's the constraints that make it slow), but we need to handle the case where A has zeros. I think precomputing sum(iszero, A; dims=1 or 2) might enable this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we're need both. It would be somewhat surprising to me if going from an O(mn) to O(m+n) size objective didn't speed things up a bit.

@timholy
Copy link
Copy Markdown
Member

timholy commented Mar 24, 2026

Clearly I need more test coverage for the A[i, j] = 0 case...try this:

julia> A = [
       8 20 18;
       20 10 0;
       18 0 5
       ];

julia> a = symcover_lmin(A)
3-element Vector{Float64}:
 6.324555320336757
 3.1622776601683795
 2.8460498941515415

julia> cover_lobjective(a, A)
2.0918640616783923

julia> a = symcover_lmin_bad(A)
3-element Vector{Float64}:
 8.04984471899924
 3.1622776601683795
 2.23606797749979

julia> cover_lobjective(a, A)
2.574290210922684

where symcover_lmin_bad is the implementation here.

@oscardssmith
Copy link
Copy Markdown
Contributor Author

Now this recovers the correct cover.

@timholy timholy merged commit ee03f45 into HolyLab:main Mar 25, 2026
5 checks passed
@timholy
Copy link
Copy Markdown
Member

timholy commented Mar 25, 2026

Thanks! This is great!

timholy added a commit that referenced this pull request Mar 25, 2026
timholy added a commit that referenced this pull request Mar 25, 2026
@oscardssmith oscardssmith deleted the patch-1 branch March 25, 2026 12:40
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