Remove symbol in object dictionary with deletion of object

[blegat]

Currently failing because hash does not work on DenseAxisArray.

Closes #1956

[codecov]

Codecov Report

Merging #2232 into master will decrease coverage by 0.10%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
- Coverage   91.31%   91.20%   -0.11%     
==========================================
  Files          42       42              
  Lines        4261     4174      -87     
==========================================
- Hits         3891     3807      -84     
+ Misses        370      367       -3     

Impacted Files	Coverage Δ
src/JuMP.jl	76.50% <91.66%> (+1.50%)	⬆️
src/constraints.jl	91.97% <100.00%> (+1.45%)	⬆️
src/copy.jl	92.00% <100.00%> (+4.50%)	⬆️
src/macros.jl	92.65% <100.00%> (-0.45%)	⬇️
src/variables.jl	96.13% <100.00%> (-0.71%)	⬇️
src/Containers/Containers.jl	50.00% <0.00%> (-38.89%)	⬇️
src/operators.jl	88.35% <0.00%> (-2.81%)	⬇️
src/Containers/DenseAxisArray.jl	83.33% <0.00%> (-1.97%)	⬇️
src/print.jl	86.18% <0.00%> (-0.81%)	⬇️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaa22fa...87968e6. Read the comment docs.

[mlubin]

How does this work for collections of variables, especially if you delete them one-by-one? This was the use case cited in #1956.

[blegat]

How does this work for collections of variables, especially if you delete them one-by-one?

If you delete each entry of a container separately then you can do unregister(model, con) to remove the entry associated to the container in the object_dictionary.

[mlubin]

If you multiply each entry of a container separately then you can do unregister(model, con) to remove the entry associated to the container in the object_dictionary.

What's the point of having an automatic unregister that only works in the corner case of scalar variables? What about making it always manual?

[blegat]

What's the point of having an automatic unregister that only works in the corner case of scalar variables? What about making it always manual?

The user could also delete the container as a whole instead of deleting each separate constraint.
Given the code

@constraint(model, con[...], ...)
for ...
    delete(model, con[...])
end

We can say "use delete(model, con)" instead of saying "use unregister(model, con).

[mlubin]

Do we currently have specialized delete method for every type of container? So delete(model, con) would work but delete.(model, con) wouldn't work? I'm not in favor of implementing magic if it's flaky and only works if you use it in a non-obvious way.

unregister(model, :x) is super obvious that it deletes the symbol from the model. Do we need to make it more complicated?

[odow]

👍 in favor of unregister(model, :x). Or even delete(model, :x) which, in addition to unregistering, will delete any elements of the container.

[blegat]

We could have both and delete(model, name::Symbol) = unregister(model, name); delete(model, model[name]).

[odow]

Why have to ways to do the same thing?

We don't want people to go

model = Model()
@variable(model, x)
unregister(model, :x)
@variable(model, x)

Now how do I access the first x?

Better to have

model = Model()
@variable(model, x)
delete!(model, :x)
@variable(model, x)

[blegat]

Does delete!(model, :x) also delete the corresponding object? Then if it is a container, it amounts to defining delete! on all containers and it's not so different with what's implemented in this PR

[odow]

Maybe I do mean just unregister then.

[mlubin]

it amounts to defining delete! on all containers and it's not so different with what's implemented in this PR

Sort of, but delete!(model, :x) doesn't have the same subtle issue as delete!(model, x) and delete!.(model, x).

[blegat]

@odow @mlubin The new version should address your comments :)

[mlubin]

Why is this loop needed? Why are we worried about constraint_object returning the wrong value?

[blegat]

It's more for is_valid that the loop is there but I agree that we could remove it

[mlubin]

I'm a bit worried about confusion from the overloaded meaning of delete. There's a big difference between delete(m, x) and delete(m, :x) that's understated by the 1-character difference. I'd tend to prefer a different name like unregister for simply deleting the key from the object dictionary (but could be convinced otherwise).

[blegat]

To be sure we talk about the same thing, you mean unregister so that the user needs to do:

delete(m, x)
unregister(m, :x)

[mlubin]

Yes, I'm proposing delete and then an explicit unregister.

[blegat]

@odow has a counter argument against unregister in #2232 (comment)