Skip to content

Conversation

@odow
Copy link
Member

@odow odow commented Sep 22, 2019

Closes #900

@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #901 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #901      +/-   ##
==========================================
- Coverage    95.1%   95.08%   -0.03%     
==========================================
  Files          80       80              
  Lines        8518     8622     +104     
==========================================
+ Hits         8101     8198      +97     
- Misses        417      424       +7
Impacted Files Coverage Δ
src/Test/UnitTests/solve.jl 100% <100%> (ø) ⬆️
src/Utilities/mockoptimizer.jl 89.85% <100%> (-0.05%) ⬇️
src/attributes.jl 88.6% <100%> (+2.56%) ⬆️

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 c393870...6a9268f. Read the comment docs.

@blegat
Copy link
Member

blegat commented Sep 23, 2019

We should define a custom error to enforce consistency between solvers wrappers, e.g.,

struct ResultIndexBoundsError{AttrType}
    attr::AttrType
    result_count::Int
end
function check_result_index_bounds(model::ModelLike, attr)
    result_count = get(model, ResultCount())
    if !(1 <= attr.result_index <= result_count)
        throw(ResultIndexBoundsError(attr, result_count)
    end
end
Base.showerror(...) = ...

@odow
Copy link
Member Author

odow commented Sep 23, 2019

Thoughts on making a breaking change by renaming the fields from .N to .result_index?

@blegat
Copy link
Member

blegat commented Sep 23, 2019

Thoughts on making a breaking change by renaming the fields from .N to .result_index?

That would break MosekTools. We could instead create a result_index function which could be cleaner than assuming a same field name.

@odow odow merged commit 74c4288 into master Sep 24, 2019
@odow odow deleted the od/result_index branch September 24, 2019 12:51
@rschwarz
Copy link
Contributor

Is the idea here that solvers would also call MOI.check_result_index_bounds inside of the relevant get methods?

@blegat
Copy link
Member

blegat commented Oct 11, 2019

Yes, exactly, all solvers should do that. They could also call MOI.throw_if_not_valid when it's a getter for an index but we currently have not test for that

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.

Error for result_index out of bound

5 participants