Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Helpful user message for old objective syntax #1543

Merged
merged 5 commits into from
Oct 14, 2018
Merged

Helpful user message for old objective syntax #1543

merged 5 commits into from
Oct 14, 2018

Conversation

blegat
Copy link
Member

@blegat blegat commented Oct 11, 2018

Closes #1541

@blegat blegat added this to the 0.19 milestone Oct 11, 2018
@mlubin
Copy link
Member

mlubin commented Oct 11, 2018

Why do we need to provide two ways to write the same thing?

@blegat
Copy link
Member Author

blegat commented Oct 11, 2018

It is not mandatory to support @objective(model, :Min, x+1) but we need to support @objective(mode, sense, x+1) where sense is a variable whose value is :Min.
If we want to support the second one, the first one comes for free.

@codecov
Copy link

codecov bot commented Oct 11, 2018

Codecov Report

Merging #1543 into master will increase coverage by <.01%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
+ Coverage   89.47%   89.48%   +<.01%     
==========================================
  Files          27       27              
  Lines        3621     3623       +2     
==========================================
+ Hits         3240     3242       +2     
  Misses        381      381
Impacted Files Coverage Δ
src/macros.jl 87.24% <94.73%> (+0.04%) ⬆️

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 123a3bf...0979e2e. Read the comment docs.

@mlubin
Copy link
Member

mlubin commented Oct 11, 2018

@objective(mode, sense, x+1) where sense is a variable whose value is :Min.

No, this shouldn't work anymore after #1562. Sense symbols are no longer recognized. You can use MOI sense values instead.

@blegat
Copy link
Member Author

blegat commented Oct 11, 2018

Sense symbols are no longer recognized

Shouldn't we add an entry in NEWS.md about that ?

@mlubin
Copy link
Member

mlubin commented Oct 11, 2018 via email

@blegat
Copy link
Member Author

blegat commented Oct 12, 2018

@mlubin I have replace the support of :Min and :Max by helpful errors in case symbols are used.

@blegat blegat changed the title 🐛 Fix objective sense with expression Helpful user message for old objective syntax Oct 12, 2018
src/macros.jl Outdated
Return an expression whose value is an `MOI.OptimizationSense` corresponding
to `sense`. Sense is either the symbol `:Min` or `:Max`, corresponding
respectively to `MOI.MinSense` and `MOI.MaxSense` or it is another symbol,
which should be the name of a variable or expression whose value is `:Min`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This discussion about :Min and :Max is out of date now, right?

src/macros.jl Outdated
which should be the name of a variable or expression whose value is `:Min`,
`:Max` or an `MOI.OptimizationSense`.
In the last case, the expression throws an error using the `_error`
function in case the value is a symbol which is not `:Min` nor `:Max`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of date?

src/macros.jl Outdated
return :(throw_error_for_symbol_sense($_error, $expr))
end

# TODO remove for JuMP v0.20
Copy link
Member

@mlubin mlubin Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually something like this should stay, because of the MethodError principle. What about updating the error message so that it just says what's wrong instead of referring to old JuMP? For example, "Unexpected sense $value. The sense must be an OptimizatonSense or \"Min\" or \"Max\"." (Also remove the ::Symbol filter on the argument type, we want a helpful error if someone gives a sense of 10.)

@mlubin mlubin merged commit 6456740 into master Oct 14, 2018
@mlubin mlubin deleted the bl/objexpr branch October 14, 2018 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants