Skip to content

Conversation

@blegat
Copy link
Member

@blegat blegat commented Apr 26, 2022

The MOI wrapper wasn't yet aligned with the fact that we now have bridges in top of the DiffOpt models.

Closed #220

@matbesancon
Copy link
Collaborator

Why introduce submodules? Overall I find it to quickly complicate both internal and external development, I know this is common in MOI but DiffOpt is small enough to contain everything

@blegat
Copy link
Member Author

blegat commented Apr 27, 2022

We have this structure where for QP and CP, we have a Form, Model, Cache, ...
We need to give them different names to avoid clashes so we are adding references to Quadratic and Conic to differentiate them but it's a bit messy. The modules provide a namespace which makes it a lot cleaner.
Second, the DiffOpt optimizer is modular in the sense that it could work with any DiffOpt.AbstractModel subtypes that show how to differentiate for a particular model. We advertise it in the paper and the fact we can refactor cleanly with modules is a consequence of it.

Could you elaborate on why it makes things complicated ? I agree that it makes it complicated if these things are not independent but in this case, they are quite independent. We want to have a clean interface that someone can implement to add new DiffOpt.AbstractModel. Having these module makes this interface apparent and forces us to keep a clean one.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #219 (2502b59) into master (7094aed) will decrease coverage by 5.41%.
The diff coverage is 66.18%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
- Coverage   91.21%   85.79%   -5.42%     
==========================================
  Files           8       11       +3     
  Lines         842      880      +38     
==========================================
- Hits          768      755      -13     
- Misses         74      125      +51     
Impacted Files Coverage Δ
src/bridges_unused.jl 0.00% <0.00%> (ø)
src/objective_container.jl 0.00% <0.00%> (ø)
src/moi_wrapper.jl 84.46% <72.22%> (-0.94%) ⬇️
src/ConicProgram/ConicProgram.jl 94.11% <92.30%> (ø)
src/DiffOpt.jl 100.00% <100.00%> (ø)
src/QuadraticProgram/QuadraticProgram.jl 97.12% <100.00%> (ø)
src/diff_opt.jl 100.00% <100.00%> (+0.78%) ⬆️

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 7094aed...2502b59. Read the comment docs.

@matbesancon
Copy link
Collaborator

Major hassle with submodules is the import ..Foo or import .Bar but I see the point that it's not too relevant here

@blegat
Copy link
Member Author

blegat commented Apr 28, 2022

Yes, since the inclusion in the modules shorten the names, the name of the modules + name of the object does not increase too much in length

@matbesancon matbesancon requested a review from joaquimg May 2, 2022 13:14
S <: SUPPORTED_VECTOR_SETS,
}
model::Optimizer, ::Type{F}, ::Type{S},
) where {F<:MOI.AbstractFunction,S<:MOI.AbstractSet}
Copy link
Member

Choose a reason for hiding this comment

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

we might need to check both optimizer and the underlying diff_models

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping @blegat ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather add special cases for the attributes that are used with the DiffOpt.AbstractModel, that's what I just did in the last commit

@matbesancon
Copy link
Collaborator

@blegat can you check the two comments above, then good to go

@matbesancon
Copy link
Collaborator

bump

@matbesancon
Copy link
Collaborator

@blegat good to go I think? We should tag a breaking release right after

@blegat blegat merged commit e287ec2 into master May 11, 2022
@blegat blegat deleted the bl/refactor branch April 3, 2023 07: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.

MOI.NormOneCone errors

4 participants