Added Base.empty! method for JuMP.Model. #2198
Conversation
…ts. Documentation is generating and tests passing.
|
This was discussed with @odow over Discourse (https://discourse.julialang.org/t/empty-for-jump-model-or-jump-direct-model/35673). |
Codecov Report
@@ Coverage Diff @@
## master #2198 +/- ##
==========================================
+ Coverage 91.12% 91.13% +0.01%
==========================================
Files 42 42
Lines 4122 4128 +6
==========================================
+ Hits 3756 3762 +6
Misses 366 366
Continue to review full report at Codecov.
|
src/JuMP.jl
Outdated
| # inneficiencies). | ||
| MOI.empty!(model.moi_backend) | ||
| empty!(model.shapes) | ||
| empty!(model.bridge_types) |
There was a problem hiding this comment.
I would keep brodge_types unaltered to be consistent witn MOI.empty! for MOI.Bridges.LazyBridgeOptimizer
There was a problem hiding this comment.
Done. Commit message is missing a model.bridge_types after the comma because bash interpreted it as a subshell invocation (sorry) and I did not perceive until after push.
There was a problem hiding this comment.
I tested with just the tests in model.jl, those passed.
|
For consistency with MOI, I don't think we should extend |
|
I will change this (I do not know if today, because I will be traveling
without internet connection most of the day), but note that doing this has
some consequences: all occurrences of `empty!` inside JuMP must be changed
to `Base.empty!` (the same applies to future occurrences referring to
`Base.empty!`); if `JuMP.empty!` is exported (what makes sense), it will
make all code with `using JuMP` and `empty!` will error about lack of
qualification. MOI is often not expected to be imported with "using" and/or
directly by the user (most users will use the `const MOI = ...` trick to
just copy and paste code freely from other places), but JuMP is often
imported with "using" and is probably used in many codes that make use of
`empty!`.
Em qui., 12 de mar. de 2020 às 09:58, Miles Lubin <notifications@github.com>
escreveu:
… For consistency with MOI, I don't think we should extend Base.empty!.
Base.empty! is defined as:
"Remove all elements from a collection.". A JuMP model is not a collection
(it doesn't implement any sort of collection-like interface), so I don't
think this applies. We could define JuMP.empty! instead.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2198 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADLVVG4BBWHSO46YW66BBJ3RHDMADANCNFSM4LFIJTPQ>
.
|
|
I agree with @henriquebecker91. Exporting an identical symbol to base Julia is very bad. We either overload |
|
Ah right, I wasn't thinking about our auto-export. Not much we can do about Julia's weird global namespace. We can go with |
|
I understand your annoyance with this choice. I would like to point that:
* There is a `Base.isempty` and a `Base.length` for String but not a `Base.empty!` (as it is immutable). Even if it is just three methods to implement a `General Collection Interface` (https://docs.julialang.org/en/v1/base/collections/#General-Collections-1), some Types of Base do not implement it fully (but use the same qualified name), the "Fully implemented by:" section after the documentation of these
three functions implicitly acknowledge this.
* The JuMP.Model type is not from Base, so it is not type piracy at least.
* Another name may be used, like `reset_model!`, `reset!`, `empty_model!`, but the average user of JuMP will probably guess `empty!`, not qualify it and be right about their guess, rarely being negatively affected by this choice in any fashion. The intuitiveness of the name is attested by the fact that MOI use the same name (just does not have the same export policy).
* As you perceived, a middle ground is not possible with this name, as either: we export `JuMP.empty!` and break a considerable amount of code in a way that will be hard for non-CS users to exactly understand what is happening; or we add a `JuMP.empty!` but not export it, but for so we need
to break the "export all names non-prefixed-with-underscore" rule.
|
|
I will leave to you two to decide if the name will be changed and, if this is the case, to which name. The current code seems to only need changes if the function name is changed. |
|
The consensus is to not change the name. |
|
Thanks for your help @henriquebecker91. I appreciate it. |
|
Thanks for being open to my rants, @odow.
Em sex., 13 de mar. de 2020 às 14:21, Oscar Dowson <notifications@github.com>
escreveu:
… Thanks for your help @henriquebecker91
<https://github.com/henriquebecker91>. I appreciate it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2198 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADLVVG2IJKL53MZ63FKB6TLRHJTQTANCNFSM4LFIJTPQ>
.
|
Not only the method but also its documentation and tests. The documentation is generating and tests passing.