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

First pass of printing for MOI constraints. #1389

Merged
merged 3 commits into from
Jul 29, 2018
Merged

First pass of printing for MOI constraints. #1389

merged 3 commits into from
Jul 29, 2018

Conversation

mlubin
Copy link
Member

@mlubin mlubin commented Jul 29, 2018

Feels like I've untangled the printing rat's nest. JuMP ConstraintRef objects now print out the underlying constraint. There's obvious room for improvement, especially in how constraint sets print out (like, print fancy versions in IJulia). See the tests for example of what the print-out looks like. I also print the constraint name if it's non-empty.

Does not yet resolve the issue #1180 because we need to decide what model printing should look like. Based on #957 we might want to print a summary by default and have another method to get the extensive-form print-out of the model.

@IainNZ, care to review since you did most of the original printing work?

@codecov
Copy link

codecov bot commented Jul 29, 2018

Codecov Report

Merging #1389 into master will increase coverage by 0.24%.
The diff coverage is 78.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1389      +/-   ##
=========================================
+ Coverage   89.55%   89.8%   +0.24%     
=========================================
  Files          25      25              
  Lines        3429    3541     +112     
=========================================
+ Hits         3071    3180     +109     
- Misses        358     361       +3
Impacted Files Coverage Δ
src/JuMP.jl 79.25% <ø> (ø) ⬆️
src/nlp.jl 82.05% <72.72%> (+2.49%) ⬆️
src/print.jl 80.95% <78.52%> (+0.43%) ⬆️
src/macros.jl 90.24% <0%> (+0.57%) ⬆️

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 58b5ff6...ba4113e. Read the comment docs.

@IainNZ IainNZ self-requested a review July 29, 2018 04:33
Copy link
Collaborator

@IainNZ IainNZ left a comment

Choose a reason for hiding this comment

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

LGTM, seems sensible starting point.

Some stuff like aff_string and quad_string really smells fishy looking at this - something for another day.

src/print.jl Outdated

#------------------------------------------------------------------------
## Model
#------------------------------------------------------------------------
function Base.show(io::IO, m::Model) # TODO temporary
function Base.show(io::IO, model::Model) # TODO temporary
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: attach the TODO to the issue?

src/print.jl Outdated
var_name = name(v)
if !isempty(var_name)
# TODO: This is wrong if variable name constains extra "]"
return math(replace(replace(var_name,"[","_{",1),"]","}"), mathmode)
return replace(replace(var_name,"[","_{",1),"]","}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Style: spaces between arguments.

@mlubin mlubin changed the base branch from ml/constrobject to master July 29, 2018 11:27
@mlubin mlubin added the Category: Printing Related to printing label Jul 29, 2018
@mlubin mlubin merged commit fa3919f into master Jul 29, 2018
@mlubin mlubin deleted the ml/printmoi branch July 29, 2018 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Printing Related to printing
Development

Successfully merging this pull request may close these issues.

2 participants