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

[breaking] Cleanup the `Context` object: remove `id_hash`, simplify dictionaries #645

Closed ericphanson closed 2 months ago

ericphanson commented 2 months ago

We are using objectid as our hash, since there are no collisions thanks to the Julia runtime. We are using this as keys in dictionaries inside the Context object. However, an IdDict is exactly a dict that uses objectid for lookup. This also makes making new AbstractVariables a bit easier, although they still need to be mutable. (Before they had to be mutable and you had to set the id_hash yourself).

I don't expect this to necessarily have any performance implications, but IdDicts should be quite fast (since there's no hashing involved), and I think this should reduce the likelihood of bugs. E.g. I noticed in ComplexConstant we were generating the id_hash randomly (for some reason), which does have the chance of collision. We are still fairly type unstable, as before.

Breakingness

I tagged this breaking since the instructions on how to create a custom AbstractVariable have changed. However, I don't think it is very breaking in practice; if you followed the old advice and had a mutable object with an id_hash field, it should still work fine, we just no longer inspect the id_hash field. However if you had an immutable variable type, and you distinguished instances via the id_hash value, now you will get collisions. I think there are essentially 0 users of the AbstractVariable interface in the wild though, so IMO it's worth cleaning up now.

Aside on ordering

This PR has the potential to regress on https://github.com/jump-dev/Convex.jl/issues/488, since this drops the ordering from some dictionaries, and we don't have a test for #488. However, I've looked through the code, and I don't think we actually rely on dictionary iteration ordering anywhere with these dictionaries. We only iterate to retrieve results from MOI, not to add variables/constraints (since those get added during conic_form! whose order is determined by the problem formulation).

The PR that fixed #488 also changed a dictionary inside Hcat, which we definitely did use iteration order to choose the order of objects that got added to the model:

https://github.com/jump-dev/Convex.jl/blob/4f52b6fd4c446cda55e77201037ae22043da2c67/src/atoms/affine/stack.jl#L87

I suspect this was the real fix. We no longer build the hcat atom in such a complicated way, and this dictionary has been deleted as far as I can tell.

I did run the problem from #488 a couple times (recreating it from scratch) and got the same results (even the same values of the variable X1 and the objective), as well.

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 97.88%. Comparing base (84a30f2) to head (16073bf).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #645 +/- ## ========================================== + Coverage 97.86% 97.88% +0.01% ========================================== Files 88 88 Lines 5116 5108 -8 ========================================== - Hits 5007 5000 -7 + Misses 109 108 -1 ```

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

odow commented 2 months ago

I took a second look at the ordering: I concur that we do not rely on the ordering for these dicts.