search

jump-dev / Convex.jl

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

Add support for using Problem as an atom #646

Closed odow closed 2 months ago

odow commented 2 months ago

Take II at #310

See also #636

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 97.89%. Comparing base (19ab3a9) to head (2df5f43).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #646 +/- ## ======================================= Coverage 97.88% 97.89% ======================================= Files 88 88 Lines 5112 5120 +8 ======================================= + Hits 5004 5012 +8 Misses 108 108 ```

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

odow commented 2 months ago

Okay, so the benefit of writing out huber like that is simplicity: it becomes a reformulation instead of an atom.

The downside is that multiple calls to huber(x) won't be cached and re-used in the same way they would it if was an atom? And there will also be more nodes in the expression graph of the problem?

That's an interesting trade-off.

ericphanson commented 2 months ago

so I think actually they will be cached, since the Problem is an AbstractExpr, and we cache the conic_form for all AbstractExpr's. (We cache if you have the same object in multiple places, like x= huber(...) and you use x in multiple places, but not if two instances are just isequal but distinct objects, since I think that wouldn't be fully correct in some cases, and also we would need to hash which can be slow).

So I think the main limitation is that there isn't a way (currently) to promise additional DCP properties that can't be derived (like maybe it is positive or increasing or something like that but it couldn't be derived automatically), which we can "inject" with atoms by defining sign etc. There also isn't the opportunity to choose how to lower to the MOI level (with a custom new_conic_form!) which lets us do fast scalar stuff and add constraints that don't have Convex.jl-level syntax etc, though hopefully that isn't needed frequently.

odow commented 2 months ago

Added a test for caching and the array return.