Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Jul 19, 2019

It would have zero argument for RejectSolution, see #782, it would be weird to require the user to provide a tuple()

@mlubin
Copy link
Member

mlubin commented Jul 19, 2019

Wait, hold on. Aren't there MethodError concerns with this? How can JuMP expose this function and know that the caller is providing the correct number of arguments?

@blegat
Copy link
Member Author

blegat commented Jul 19, 2019

They will get an error like it's not supported:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/e458dfd55b581d2c0470a69095ab0001df823d82/src/attributes.jl#L389-L395
We have the same issue for get:
https://github.com/JuliaOpt/MathOptInterface.jl/blob/e458dfd55b581d2c0470a69095ab0001df823d82/src/attributes.jl#L248-L250
But not for set, which throws a MethodError (but still violating the MethodError principle since it's not the same one but avoiding this causes ambiguity as mentioned in the comment):
https://github.com/JuliaOpt/MathOptInterface.jl/blob/e458dfd55b581d2c0470a69095ab0001df823d82/src/attributes.jl#L348-L376

@blegat blegat added this to the v0.9 milestone Jul 19, 2019
@mlubin
Copy link
Member

mlubin commented Jul 19, 2019

They will get an error like it's not supported:

If you try to submit using a submittable that's supported, then you'll hit throw(SubmitNotAllowed(sub)). This seems like the wrong error message, because submit may be allowed, you just submitted with the wrong number of arguments.

I don't see an easy workaround if we're following the MethodError principle.

We have the same issue for get

Is this an issue with the fallback or with the definition of get? Why do we need get to allow an arbitrary number of arguments?

@blegat
Copy link
Member Author

blegat commented Jul 19, 2019

Is this an issue with the fallback or with the definition of get? Why do we need get to allow an arbitrary number of arguments?

Not, just with the fallback.

@codecov-io
Copy link

codecov-io commented Jul 19, 2019

Codecov Report

Merging #797 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #797   +/-   ##
======================================
  Coverage    94.1%   94.1%           
======================================
  Files          59      59           
  Lines        6671    6671           
======================================
  Hits         6278    6278           
  Misses        393     393
Impacted Files Coverage Δ
src/attributes.jl 85.54% <0%> (ø) ⬆️

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 e458dfd...9f15e1b. Read the comment docs.

@blegat
Copy link
Member Author

blegat commented Jul 19, 2019

In fact, it does not make sense to throw SubmitNotAllowed in the fallback since for submittable you support iff you implement submit. It should throw an methoderror now
For set, it's different since you may support an attribute but only implement allocate/load.

@mlubin
Copy link
Member

mlubin commented Jul 19, 2019

Not, just with the fallback.

What is preventing us from defining the fallback without using varargs?

@blegat blegat mentioned this pull request Jul 19, 2019
@blegat
Copy link
Member Author

blegat commented Jul 19, 2019

What is preventing us from defining the fallback without using varargs?

Nothing, let's open an issue #799

@mlubin
Copy link
Member

mlubin commented Jul 19, 2019

LGTM. Merge when ready.

@blegat blegat merged commit deec96a into master Jul 19, 2019
@odow odow deleted the bl/submit_values branch August 29, 2019 14:41
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.

3 participants