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

Improved Raw Expression NLP API #2672

Merged
merged 4 commits into from
Aug 24, 2021
Merged

Improved Raw Expression NLP API #2672

merged 4 commits into from
Aug 24, 2021

Conversation

pulsipher
Copy link
Contributor

@pulsipher pulsipher commented Aug 24, 2021

This closes #2671. This adds add_NL_expression which serves as an alternative to @NLexpression in like manner to how set_NL_objective and add_NL_constraint serve as alternatives to their macro counterparts. This also adds the capability for these to accept GenericAffExprs and GenericQuadExprs.

model = Model()
@variable(model, x)
@expression(model, aff, 2x + 4)
@expression(model, quad, x^2 - x)
expr_ref = add_NL_expression(model, :($aff ^ $x))
add_NL_constraint(model, :($quad + $expr_ref <= 0))

Edit (odow):

Closes #2506
Closes #2671

@pulsipher
Copy link
Contributor Author

pulsipher commented Aug 24, 2021

Note that the additions have been formatted via JuliaFormatter, but it appears to be failing due to an unrelated issue with something in src/lp_sensitivity2.jl

Moreover, the doc test fails since a forked repo doesn't have writing permissions.

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #2672 (afcc8bd) into master (5a73967) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2672      +/-   ##
==========================================
+ Coverage   93.51%   93.54%   +0.03%     
==========================================
  Files          44       44              
  Lines        5551     5578      +27     
==========================================
+ Hits         5191     5218      +27     
  Misses        360      360              
Impacted Files Coverage Δ
src/nlp.jl 93.46% <100.00%> (+0.01%) ⬆️
src/parse_nlp.jl 96.34% <100.00%> (+0.41%) ⬆️

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 5a73967...afcc8bd. Read the comment docs.

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Not sure what happened to the formatter? Doc failure isn't your fault.

docs/src/manual/nlp.md Show resolved Hide resolved
@pulsipher
Copy link
Contributor Author

Not sure what happened to the formatter?

Ya this appears to be affecting the master branch and the other pending pull requests as well.

@odow
Copy link
Member

odow commented Aug 24, 2021

The latest formatting run on master passed: https://github.com/jump-dev/JuMP.jl/runs/3391118073

@odow
Copy link
Member

odow commented Aug 24, 2021

Can you run the formatter locally and see what happens?

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

LGTM, contingent on the formatting failure.

Docs pass but fail to deploy due to the bug in documenter.

@odow
Copy link
Member

odow commented Aug 24, 2021

This also closes #2506?

@pulsipher
Copy link
Contributor Author

pulsipher commented Aug 24, 2021

This also closes #2506?

I believe it does!

Can you run the formatter locally and see what happens?

It reproduces locally as well. After trying each source, I deduced the problem lies with checking src/lp_sensitivity.jl which is strange since my pull request doesn't touch that file.

julia> format("./src/lp_sensitivity.jl")
ERROR: Parsing error for input occurred on line 419, offset: 79
Stacktrace:
  [1] error(s::String)
    @ Base .\error.jl:33
  [2] format_text(text::String, style::DefaultStyle, opts::JuliaFormatter.Options)
    @ JuliaFormatter C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:322
  [3] #format_text#161
    @ C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:316 [inlined]
  [4] #format_text#160
    @ C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:310 [inlined]
  [5] format_file(filename::String; overwrite::Bool, verbose::Bool, format_markdown::Bool, format_options::Base.Iterators.Pairs{Symbol, Integer, NTuple{5, Symbol}, NamedTuple{(:short_to_long_function_def, :remove_extra_newlines, :margin, :always_for_in, :always_use_return), Tuple{Bool, Bool, Int64, Bool, Bool}}})
    @ JuliaFormatter C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:421
  [6] format(paths::Tuple{String}; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, 
NamedTuple{(), Tuple{}}})
    @ JuliaFormatter C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:528
  [7] format
    @ C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:504 [inlined]
  [8] #format#170
    @ C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:553 [inlined]
  [9] format(path::String)
    @ JuliaFormatter C:\Users\pulsipher\.julia\packages\JuliaFormatter\FlpTt\src\JuliaFormatter.jl:553
 [10] top-level scope
    @ REPL[7]:1

Moreover, I don't see anything wrong with that line of code: https://github.com/pulsipher/JuMP.jl/blob/afcc8bd0f46a3fce28508e316a6d4a037b1ad5a0/src/lp_sensitivity.jl#L419

@odow
Copy link
Member

odow commented Aug 24, 2021

I can't reproduce this on Mac

(@v1.6) pkg> st JuliaFormatter
      Status `~/.julia/environments/v1.6/Project.toml`
  [98e50ef6] JuliaFormatter v0.13.2

julia> using JuliaFormatter

julia> format("src/lp_sensitivity.jl")
true

julia> format("src"; verbose = true)
Formatting src/JuMP.jl
Formatting src/aff_expr.jl
Formatting src/callbacks.jl
Formatting src/complement.jl
Formatting src/constraints.jl
Formatting src/copy.jl
Formatting src/feasibility_checker.jl
Formatting src/file_formats.jl
Formatting src/indicator.jl
Formatting src/lp_sensitivity.jl
Formatting src/lp_sensitivity2.jl
Formatting src/macros.jl
Formatting src/mutable_arithmetics.jl
Formatting src/nlp.jl
Formatting src/objective.jl
Formatting src/operators.jl
Formatting src/optimizer_interface.jl
Formatting src/parse_nlp.jl
Formatting src/precompile.jl
Formatting src/print.jl
Formatting src/quad_expr.jl
Formatting src/sd.jl
Formatting src/sets.jl
Formatting src/shapes.jl
Formatting src/utils.jl
Formatting src/variables.jl
Formatting src/Containers/Containers.jl
Formatting src/Containers/DenseAxisArray.jl
Formatting src/Containers/SparseAxisArray.jl
Formatting src/Containers/container.jl
Formatting src/Containers/generate_container.jl
Formatting src/Containers/macro.jl
Formatting src/Containers/nested_iterator.jl
Formatting src/Containers/no_duplicate_dict.jl
Formatting src/Containers/vectorized_product_iterator.jl
Formatting src/_Derivatives/_Derivatives.jl
Formatting src/_Derivatives/coloring.jl
Formatting src/_Derivatives/conversion.jl
Formatting src/_Derivatives/forward.jl
Formatting src/_Derivatives/linearity.jl
Formatting src/_Derivatives/reverse.jl
Formatting src/_Derivatives/sparsity.jl
Formatting src/_Derivatives/subexpressions.jl
Formatting src/_Derivatives/topological_sort.jl
Formatting src/_Derivatives/types.jl
true

@odow
Copy link
Member

odow commented Aug 24, 2021

I'm going to merge this since I've tested locally. If it passes on master, it might be a Windows issue, or something with pulsiphers fork.

@odow odow merged commit 2e23b5e into jump-dev:master Aug 24, 2021
@pulsipher
Copy link
Contributor Author

pulsipher commented Aug 24, 2021

Hmm, this seems to propagate to master now in like manner to https://github.com/pulsipher/JuMP.jl/runs/3415921746, So it appears that something has changed in lp_sensitivity.jl that is not readily being recognized by git. When I remove the file format("src") works, but when I put it back it errors again. I even tried deleting it and replacing it with the one from the repo previous to this pull request to no avail...

@odow
Copy link
Member

odow commented Aug 24, 2021

I can reproduce this on linux. I'll take a look

@odow odow mentioned this pull request Aug 24, 2021
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.

Extend Raw NLP Expr Input to Allow AffExprs and QuadExprs Add add_NL_expression function
2 participants