Skip to content

Fix setter for ConstraintSet and ConstraintFunction#120

Closed
rafabench wants to merge 1 commit intomasterfrom
rb/fix_constraint_obj
Closed

Fix setter for ConstraintSet and ConstraintFunction#120
rafabench wants to merge 1 commit intomasterfrom
rb/fix_constraint_obj

Conversation

@rafabench
Copy link
Copy Markdown
Collaborator

Close #119
Follows the same idea of #109

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 30, 2022

Codecov Report

Base: 95.14% // Head: 93.99% // Decreases project coverage by -1.14% ⚠️

Coverage data is based on head (e87ec9a) compared to base (e3c7cb3).
Patch coverage: 40.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
- Coverage   95.14%   93.99%   -1.15%     
==========================================
  Files           4        4              
  Lines         968     1033      +65     
==========================================
+ Hits          921      971      +50     
- Misses         47       62      +15     
Impacted Files Coverage Δ
src/ParametricOptInterface.jl 89.72% <40.00%> (-2.27%) ⬇️
src/utils.jl 100.00% <0.00%> (ø)
src/update_parameters.jl 99.02% <0.00%> (+0.11%) ⬆️
src/duals.jl 98.63% <0.00%> (+0.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Copy Markdown
Member

@joaquimg joaquimg left a comment

Choose a reason for hiding this comment

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

there are more detail that need to be considered.

  • outer indices must be converted to inner indices
  • new function might be adding parameters do existing constraints
  • new function might be removing parameters from existing constraints

f::F,
) where {T,S<:MOI.AbstractSet,F}
if haskey(model.affine_added_cache, c)
MOI.set(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this looks too simple, don't we have any other data cached for affine function in the POI.Optimizer?

f::F,
) where {T,S<:MOI.AbstractSet,F}
if haskey(model.quadratic_added_cache, c)
MOI.set(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same as above

s::S,
) where {T,S<:MOI.AbstractSet}
if haskey(model.quadratic_added_cache, c)
MOI.set(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

s::S,
) where {T,S<:MOI.AbstractSet}
if haskey(model.affine_added_cache, c)
MOI.set(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

f,
)
else
MOI.set(model.optimizer, MOI.ConstraintFunction(), c, f)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is also strange, you might have parameters in f

@joaquimg
Copy link
Copy Markdown
Member

Isn't #119 related to get methods? why are you changing set methods?

@jd-lara
Copy link
Copy Markdown
Contributor

jd-lara commented Aug 30, 2022

Isn't #119 related to get methods? why are you changing set methods?

It looks like the issue is that the values aren't being set to begin with.

@rafabench
Copy link
Copy Markdown
Collaborator Author

Isn't #119 related to get methods? why are you changing set methods?

Probably we will need to fix both getters and setters methods.

@jd-lara
Copy link
Copy Markdown
Contributor

jd-lara commented Aug 31, 2022

Maybe this is the correct place to fix this but why return after a throw?

function MOI.get(
    model::Optimizer,
    attr::MOI.ConstraintSet,
    ci::MOI.ConstraintIndex{F,S},
) where {F,S}
    if haskey(model.original_constraint_function_and_set_cache, ci)
        return model.original_constraint_function_and_set_cache[ci][2]
    else
        MOI.throw_if_not_valid(model, ci)
        return MOI.get(model.optimizer, attr, ci)
    end
end

@odow
Copy link
Copy Markdown
Member

odow commented Aug 31, 2022

but why return after a throw?

The throw only happens if the index is not valid.

@jd-lara
Copy link
Copy Markdown
Contributor

jd-lara commented Aug 31, 2022

Yeah right, I realized it doesn't always throw.

@jd-lara
Copy link
Copy Markdown
Contributor

jd-lara commented Nov 9, 2022

@joaquimg do you know if there is progress on this PR? I am ready to continue with the integration in PowerSimulations.jl

@guilhermebodin
Copy link
Copy Markdown
Collaborator

@jd-lara could you share the code that triggers the problem?

@joaquimg
Copy link
Copy Markdown
Member

replaced by #130

@joaquimg joaquimg closed this Apr 17, 2023
@odow odow deleted the rb/fix_constraint_obj branch April 20, 2023 07:14
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.

JuMP.constraint_object fails when using direct_mode

5 participants