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

[ericphanson]

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 #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:

Convex.jl/src/atoms/affine/stack.jl

Line 87 in 4f52b6f

for (id, col_size) in variable_to_sizes

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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     

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

[odow]

Seems okay. I don't know if I truly understand the implications, but it looks sensible.

[odow]

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