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

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

Merged
merged 9 commits into from Nov 21, 2021
Merged

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

merged 9 commits into from Nov 21, 2021

Conversation

chriscoey
Copy link
Contributor

@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
Copy link
Member

odow commented Nov 20, 2021

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
Copy link
Contributor Author

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
Copy link
Member

odow commented Nov 20, 2021

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
Copy link
Contributor Author

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
Copy link
Member

odow commented Nov 20, 2021

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).

@chriscoey chriscoey changed the title WIP: cleanup tests and start on new MOI.Test tests clean up somewhat and use new MOI.Test tests Nov 21, 2021
@chriscoey chriscoey mentioned this pull request Nov 21, 2021
@codecov
Copy link

codecov bot commented Nov 21, 2021

Codecov Report

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

❗ 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%) ⬇️
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.

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.

I haven't had a look through, but you may as well merge if the tests are passing. I'll take a better look this week once I've made the changes the MOI.

@chriscoey chriscoey mentioned this pull request Nov 21, 2021
@chriscoey chriscoey merged commit 1d32e6c into master Nov 21, 2021
@odow odow deleted the moitest branch November 21, 2021 21:27
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.

None yet

2 participants