Expose JuMP.delete and JuMP.is_valid methods #1445

odow · 2018-08-27T06:22:34Z

I have prefixed JuMP.delete in the code to disambiguate Base.delete! as it was slightly confusing distinguishing between delete and delete!.

Maybe this is a good candidate to break from the style guide and extend a Base method?

~~There will be some conflicts with #1442~~

MOI.delete! and MOI.isvalid for JuMP variables and constraints.

codecov · 2018-08-27T07:35:01Z

Codecov Report

Merging #1445 into master will increase coverage by 0.26%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
+ Coverage   88.86%   89.13%   +0.26%     
==========================================
  Files          25       25              
  Lines        3692     3700       +8     
==========================================
+ Hits         3281     3298      +17     
+ Misses        411      402       -9

Impacted Files	Coverage Δ
src/variables.jl	`93.85% <100%> (+5.48%)`	⬆️
src/JuMP.jl	`77.37% <100%> (+0.16%)`	⬆️

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 9835373...4495f2a. Read the comment docs.

mlubin

The diff has only 50% coverage, which isn't great. I think the delete_* and unset_* methods might not be tested?

mlubin · 2018-08-27T23:59:35Z

docs/src/variables.md

@@ -470,7 +470,21 @@ Dict{Symbol,Symmetric{JuMP.VariableRef,Array{JuMP.VariableRef,2}}} with 2 entrie

 ## Deleting variables

-**TODO(@odow):** explain how to delete variables.
+Finally, JuMP supports the deletion of optimization variables. To delete


"Finally" won't age well if we move text around or add new sections.

mlubin · 2018-08-28T00:01:11Z

docs/src/variables.md

@@ -470,7 +470,21 @@ Dict{Symbol,Symmetric{JuMP.VariableRef,Array{JuMP.VariableRef,2}}} with 2 entrie

 ## Deleting variables

-**TODO(@odow):** explain how to delete variables.
+Finally, JuMP supports the deletion of optimization variables. To delete
+variables, we can use the `JuMP.delete` method. We can also check whether `x`


What happens when the variable that's deleted already appears in constraints?

Urgh. Solver-specific? At least in direct mode. What does the caching optimizer do? @blegat?

mlubin · 2018-08-28T00:02:24Z

src/JuMP.jl

+Delete the constraint associated with `constraint_ref` from the model `model`.
+"""
+function delete(model::Model, constraint_ref::ConstraintRef{Model})
+    # TODO: should model be a parameter in the constraint_ref?


Model is already a parameter in the constraint ref. Not sure what this TODO means, maybe just delete?

mlubin · 2018-08-28T00:03:00Z

src/variables.jl

+Delete the variable associated with `variable_ref` from the model `model`.
+"""
+function delete(model::Model, variable_ref::VariableRef)
+    @assert model === variable_ref.m


Prefer error messages over asserts for validating user-provided arguments.

mlubin · 2018-08-28T00:03:34Z

src/variables.jl

+function delete_lower_bound(variable_ref::VariableRef)
+    JuMP.delete(variable_ref.m, LowerBoundRef(variable_ref))
+    delete!(variable_ref.m.variable_to_lower_bound, index(variable_ref))
+    return nothing


Nit: return instead of return nothing (also below).

mlubin · 2018-08-28T00:10:50Z

I see the issue with JuMP.delete vs. Base.delete!.
The docstring for Base.delete! says:

delete!(collection, key)

  Delete the mapping for the given key in a collection, and return the collection.

Can we plausibly say that this applies to deleting a variable from a JuMP model? Also and return the collection means that we should return the model?

codecov · 2018-08-28T00:14:09Z

Codecov Report

Merging #1445 into master will increase coverage by <.01%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
+ Coverage   88.86%   88.87%   +<.01%     
==========================================
  Files          25       25              
  Lines        3692     3694       +2     
==========================================
+ Hits         3281     3283       +2     
  Misses        411      411

Impacted Files	Coverage Δ
src/JuMP.jl	`77.2% <100%> (ø)`	⬆️
src/variables.jl	`88.5% <50%> (+0.13%)`	⬆️

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 9835373...c7e7e24. Read the comment docs.

odow · 2018-08-28T03:02:06Z

Can we plausibly say that this applies to deleting a variable from a JuMP model?

Yes, but for consistency, I would prefer that the JuMP. prefix is for every user-facing JuMP function. It is a simple rule to remember.

blegat · 2018-08-28T06:47:08Z

The decision we take for JuMP should be the same than for MOI. I don't see any different between the JuMP model/references and MOI model/indices cases. Except that we care more about user-friendliness with JuMP so that might be an argument for Base.delete!

odow · 2018-08-28T11:07:51Z

Okay, this diff coverage is now 100% so we're good to merge from my point of view.

mlubin · 2018-08-29T01:35:58Z

We're missing a style guide point on when it's appropriate to add methods to existing functions. I would say that it should be one concept per function, and you shouldn't just add a method because of the name. One way to check this is "can the new method be described by the existing docstring?"

Deleting a variable from a model is arguably not the same as removing an item from a collection (models aren't iterable), so that's an argument for not using Base.delete!. I agree we should be consistent between MOI and JuMP now that we have style guidelines.

mlubin · 2018-08-29T01:39:19Z

test/JuMPExtension.jl

-    delete!(m.variables, vref.idx)
-    delete!(m.varnames, vref.idx)
+function JuMP.delete(model::MyModel, variable_ref::MyVariableRef)
+    @assert JuMP.is_valid(model, variable_ref)


Nit: Error message instead of assert.

I think I just made it an assert because it is a test for an extension. Should this be a style thing to never use asserts and always have an informative error message?

using errors in JuMPExtension is maybe overkill

Oops, I got confused. It's fine to use this in a test.

odow and others added 2 commits August 27, 2018 18:17

Expose JuMP.delete and JuMP.is_valid methods as a wrapper around

0a3d8ed

MOI.delete! and MOI.isvalid for JuMP variables and constraints.

Add newlines to docs

7e0973f

blegat approved these changes Aug 27, 2018

View reviewed changes

Merge branch 'master' into od/delete

c7e7e24

mlubin reviewed Aug 28, 2018

View reviewed changes

odow added 2 commits August 28, 2018 16:12

Improve test coverage and style fixes

c78aeb1

Improve test coverage

4495f2a

odow mentioned this pull request Aug 28, 2018

What happens when deleting a variable in a constraint? jump-dev/MathOptInterface.jl#511

Closed

1 task

mlubin reviewed Aug 29, 2018

View reviewed changes

mlubin merged commit f5a9fe9 into master Aug 29, 2018

mlubin deleted the od/delete branch August 29, 2018 11:32

odow mentioned this pull request Sep 4, 2018

[WIP] Add solutions for tests JuliaOpt/LinQuadOptInterface.jl#57

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose JuMP.delete and JuMP.is_valid methods #1445

Expose JuMP.delete and JuMP.is_valid methods #1445

odow commented Aug 27, 2018 •

edited

Loading

codecov bot commented Aug 27, 2018 •

edited

Loading

mlubin left a comment

mlubin Aug 27, 2018

mlubin Aug 28, 2018

odow Aug 28, 2018

mlubin Aug 28, 2018

mlubin Aug 28, 2018

mlubin Aug 28, 2018

mlubin commented Aug 28, 2018

codecov bot commented Aug 28, 2018

odow commented Aug 28, 2018

blegat commented Aug 28, 2018

odow commented Aug 28, 2018

mlubin commented Aug 29, 2018

mlubin Aug 29, 2018

odow Aug 29, 2018

blegat Aug 29, 2018

mlubin Aug 29, 2018

Expose JuMP.delete and JuMP.is_valid methods #1445

Expose JuMP.delete and JuMP.is_valid methods #1445

Conversation

odow commented Aug 27, 2018 • edited Loading

codecov bot commented Aug 27, 2018 • edited Loading

Codecov Report

mlubin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin commented Aug 28, 2018

codecov bot commented Aug 28, 2018

Codecov Report

odow commented Aug 28, 2018

blegat commented Aug 28, 2018

odow commented Aug 28, 2018

mlubin commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow commented Aug 27, 2018 •

edited

Loading

codecov bot commented Aug 27, 2018 •

edited

Loading