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

Add reformulation parser #745

Merged
merged 8 commits into from
Nov 25, 2022
Merged

Add reformulation parser #745

merged 8 commits into from
Nov 25, 2022

Conversation

eduardovegas
Copy link
Collaborator

Closes #702 .

@eduardovegas eduardovegas requested a review from guimarqu November 3, 2022 03:14
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 86.54% // Head: 86.54% // No change to project coverage 👍

Coverage data is based on head (d865a97) compared to base (810655b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #745   +/-   ##
=======================================
  Coverage   86.54%   86.54%           
=======================================
  Files          59       59           
  Lines        5405     5405           
=======================================
  Hits         4678     4678           
  Misses        727      727           

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.

@guimarqu
Copy link
Contributor

Do you think you can move this outside Coluna? I think it would be nice to keep it in the tests. I don't know if we can have a kind of TestUtils module in the test folder or something else similar.

@eduardovegas
Copy link
Collaborator Author

Would it be better if the parser file in src/ was moved to test/, and add include("parser.jl") in ColunaTests module? Because creating a submodule I'd have to define some constants that are already in ColunaTests and export the functions from the new module.

@guimarqu
Copy link
Contributor

Would it be better if the parser file in src/ was moved to test/, and add include("parser.jl") in ColunaTests module? Because creating a submodule I'd have to define some constants that are already in ColunaTests and export the functions from the new module.

Sounds good :)

@eduardovegas eduardovegas marked this pull request as ready for review November 16, 2022 16:25
Comment on lines 9 to 13
names = []
kinds = []
duties = []
costs = []
bounds = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
names = []
kinds = []
duties = []
costs = []
bounds = []
names = String[]
kinds = ClMP.VarKind[]
duties = ClMP.Duty{ClMP.Variable}[]
costs = Float64[]
bounds = Tuple{Float64,Float64}[]

Copy link
Contributor

@guimarqu guimarqu left a comment

Choose a reason for hiding this comment

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

This is a nice PR. Some minor style issues to address.

infos = AuxiliaryConstrInfo[]
coeff_matrix = CL.getcoefmatrix(form)
for (constrid, constr) in CL.getconstrs(form)
coeffs = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
coeffs = []
coeffs = Float64[]

y1 >= 1
1 <= y2
"""
@test_throws ErrorException reformfromstring(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a UndefinedMasterParserError thrown when this error happens?

y1 >= 1
1 <= y2
"""
@test_throws ErrorException reformfromstring(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a specific error type e.g. UndefinedObjectiveParserError

bounds
y <= 10
"""
@test_throws ErrorException reformfromstring(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you create a specific error type?

bounds
x >= 10
"""
@test_throws ErrorException reformfromstring(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you create a specific error type?

test/parser.jl Outdated
Comment on lines 172 to 179
if m !== nothing
vars = _get_vars_list(m[4]) # separate variables as "x_1, x_2" into a list [x_1, x_2]
if m[1] !== nothing # has lower bound (nb <= var) or upper bound (nb >= var)
bound1 = String(m[2])
end
if m[5] !== nothing # has upper bound (var <= nb) or lower bound (var >= nb)
bound2 = String(m[6])
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if m !== nothing
vars = _get_vars_list(m[4]) # separate variables as "x_1, x_2" into a list [x_1, x_2]
if m[1] !== nothing # has lower bound (nb <= var) or upper bound (nb >= var)
bound1 = String(m[2])
end
if m[5] !== nothing # has upper bound (var <= nb) or lower bound (var >= nb)
bound2 = String(m[6])
end
if !isnothing(m)
vars = _get_vars_list(m[4]) # separate variables as "x_1, x_2" into a list [x_1, x_2]
if !isnothing(m[1]) # has lower bound (nb <= var) or upper bound (nb >= var)
bound1 = String(m[2])
end
if !isnothing(m[5]) # has upper bound (var <= nb) or lower bound (var >= nb)
bound2 = String(m[6])
end

test/parser.jl Outdated

function read_master!(::Val{:constraints}, cache::ReadCache, line::AbstractString)
constr = _read_constraint(line)
if constr !== nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if constr !== nothing
if !isnothing(constr)

test/parser.jl Outdated

function read_subproblem!(cache::ReadCache, line::AbstractString, nb_sp::Int64)
constr = _read_constraint(line)
if constr !== nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if constr !== nothing
if !isnothing(constr)

test/parser.jl Outdated
if haskey(cache.variables, varid)
var = cache.variables[varid]
if var.duty <= ClMP.DwSpPricingVar || var.duty <= ClMP.MasterRepPricingVar
if spform === nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if spform === nothing
if isnothing(spform)

push!(constraints, c)
i += 1
end
#create subproblems constraints in master
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very long method. Is it possible to isolate each step in a specific method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor

@guimarqu guimarqu left a comment

Choose a reason for hiding this comment

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

Thanks @eduardovegas!

@guimarqu guimarqu merged commit c419e30 into master Nov 25, 2022
@guimarqu guimarqu deleted the adding_reformulation_parser branch November 25, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model parser for tests
2 participants