jump-dev / Convex.jl

A Julia package for disciplined convex programming
https://jump.dev/Convex.jl/stable/
Other
567 stars 121 forks source link

change `LogSumExp` atom to be per-column, for efficient `logisticloss` #685

Closed ericphanson closed 4 months ago

ericphanson commented 4 months ago

closes #682

the actual atom was never documented, only logsumexp, so this should be non-breaking

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.20%. Comparing base (2978c32) to head (73ce7c1). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #685 +/- ## ======================================= Coverage 98.19% 98.20% ======================================= Files 89 89 Lines 5162 5189 +27 ======================================= + Hits 5069 5096 +27 Misses 93 93 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odow commented 4 months ago

Since this is a non-breaking performance issue, I think I'd prefer to come up with a standard way to represent dims arguments. So dims = 0 is element-wise, dims = 1 makes a row vector, and dims = 2 makes a col vector.

We have quite a few hodgepodge elementwise/row/column conventions. It'd be useful to standardize.

ericphanson commented 4 months ago

I wonder if there's a lil abstraction we use, like

Base.eachcol(x::AbstractExpr) = EachColAtom(x)
Base.eachrow(x::AbstractExpr) = EachRowAtom(x)

and then these have slow IndexAtom based fallbacks, but we can sneak in optimizations like logsumexp.(::EachColAtom) lowering to something nice. (Either literally using broadcasting and hooking into the machinery there, or using map and regular dispatch etc).

Base.map(::typeof(logsumexp), cols::EachColAtom) = ...partially specified problem with fast implementation...
odow commented 4 months ago

Yeah, I don't have a solution off the top of my head, but something like that.

This is my main bugbear: https://github.com/jump-dev/Convex.jl/blob/bce84a9620b093fc95a4c0daf9262d77f0f73aab/src/atoms/EntropyAtom.jl#L34-L36

ericphanson commented 4 months ago

closing in favor of #692