-
Notifications
You must be signed in to change notification settings - Fork 18
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
Revamp for Julia v0.6 #66
Conversation
Please drop 0.5! |
I might put some effort into sorting out the tests while I'm at it. |
src/AmplNLWriter.jl
Outdated
"Pkg.add(\"CoinOptServices\")") | ||
AmplNLSolver(CoinOptServices.bonmin, options; filename=filename) | ||
error(""" | ||
BonminNLSolver is no longer available by default through AmplNLWriter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big breaking change and needs corresponding updates to the README and JuMP's solver table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not planning on merging this any time soon. Just trying it out. Yay/Nay on whether this is a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it. This dependency is very flaky.
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
======================================
Coverage ? 0%
======================================
Files ? 4
Lines ? 644
Branches ? 0
======================================
Hits ? 0
Misses ? 644
Partials ? 0
Continue to review full report at Codecov.
|
This is probably ready to be merged. Once it is merged, I might go through and have another tidy up. Then we should tag a new version in METADATA and merge the JuMP PR to reflect the changes in the solver table. |
Merging this so that we can make progress with other PRs. Cc @ccoffrin |
This PR should probably be split into some smaller ones
Allowing .sol files to be read from an IO stream
I'm playing around with the NL file-format interface to NEOS (ref https://neos-server.org/neos/solvers/milp:CPLEX/NL.html and jump-dev/NEOSServer.jl#17).
I made some changes so I can read the .sol file from a stream instead of having to write the file to disk.
Changed tests to
@testset
I've also removed CoinOptServices from the testing suite as it needs some work and was causing failures on my windows machine.
Dumping the XXXNLSolver
I've removed
BonminNLSolver
,CouenneNLSolver
, andIpoptNLSolver
(and therefore the conditional dependencies) in favour of forcing the user to explicitly construct the solvers.becomes
which is a bit more clear and less magical about what is actually occuring.
This is a hard breaking change for current users.
Hopefully this fixes #65
Todo before merge
Removed support for v0.5
This uses Julia v0.6 syntax (.!
) but doesn't change the REQUIRE yet so still todo:[ ] fix deprecation properly by bumping version or using compatCloses #64
Better error message
If a solver crashes without producing a
.sol
file the user gets a slightly more helpful error message. Closes #67Added codecov
Closes #69