search

jump-dev / Pavito.jl

A gradient-based outer approximation solver for convex mixed-integer nonlinear programming (MINLP)
Other
60 stars 10 forks source link

clean up somewhat and use new MOI.Test tests #43

Closed chriscoey closed 2 years ago

chriscoey commented 2 years ago

@blegat @odow what is the simplest way to select the right set of MOI.Test functions to include/exclude? With the first commit here, there are dozens of MOI test functions that fail - too many to list by hand. Many fail because Pavito errors on instances without integer/discrete constraints, but I don't know a simple way to exclude all instances without such constraints.

odow commented 2 years ago

Many fail because Pavito errors on instances without integer/discrete constraints

We don't have a way of doing this because it's a pretty uncommon ask. You'd have to manually exclude them. you can also partially match function names. So you could have exclude = ["test_nonlinear_"] to exclude all nonlinear-related tests.

chriscoey commented 2 years ago

Thanks. Given how many need to be excluded, could it make more sense to just list the ones to include? I assume that is what the "include = ..." is for. Alternatively, I could make pavito not error when there are no discrete constraints, and just call the continuous solver. Not sure what makes the most sense.

odow commented 2 years ago

could it make more sense to just list the ones to include

Yes, that's what include is for.

I could make pavito not error when there are no discrete constraints, and just call the continuous solver.

This seems like the way to go.

chriscoey commented 2 years ago

Ok I'll try that. And should I do the module wrapping of MOI tests that I think I've seen in some other packages recently? Or is that not really important?

odow commented 2 years ago

It's not necessary, but putting thing in a module gets rid of a bunch of scoping and global variable issues (it's too common for tests to accidentally rely on data/variables set in other parts (or files) or the tests).

codecov[bot] commented 2 years ago

Codecov Report

Merging #43 (56d358d) into master (7847640) will increase coverage by 15.93%. The diff coverage is 84.28%.

:exclamation: Current head 56d358d differs from pull request most recent head 2bcf562. Consider uploading reports for the commit 2bcf562 to get more accurate results Impacted file tree graph

@@             Coverage Diff             @@
##           master      #43       +/-   ##
===========================================
+ Coverage   69.82%   85.76%   +15.93%     
===========================================
  Files           3        4        +1     
  Lines         570      583       +13     
===========================================
+ Hits          398      500      +102     
+ Misses        172       83       -89
Impacted Files Coverage Δ
src/MOI_wrapper.jl 82.20% <75.47%> (-0.31%) :arrow_down:
src/optimize.jl 83.88% <83.88%> (ø)
src/cut_utils.jl 94.56% <94.56%> (ø)
src/infeasible_nlp.jl 100.00% <100.00%> (ø)

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 7847640...2bcf562. Read the comment docs.