-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support more quadratic solvers #2574
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
src/solver/optimisation/opt_pilotage_optimisation_quadratique.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
Signed-off-by: Peter Mitri <[email protected]>
@@ -48,7 +48,10 @@ namespace | |||
{ | |||
void printSolvers() |
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.
In the scope of this PR ? (not sure)
printSolvers() :
- Should be renamed into : logAllAvailableSolvers()
- why do we print to std::cout and not in usual logs ? Does it even make sense ?
Moreover printSolvers is called from handleOptions which has a very non expressive and probably wrong name and, besides printing, does very strange things.
src/tests/end-to-end/binding_constraints/test_binding_constraints.cpp
Outdated
Show resolved
Hide resolved
INCLUDE | ||
"${CMAKE_SOURCE_DIR}/solver/utils" |
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.
INCLUDE
"${CMAKE_SOURCE_DIR}/solver/utils"
is this useful ?
I mean : if we link with Antares::solverUtils, related include directory should be automatic
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.
without it, test file "basis_status.cpp" cannot find "basis_status_impl.h" which is located under src/solver/utils (with sources, not with headers in include folder)
|
||
BOOST_FIXTURE_TEST_CASE(solver_not_supported, QpFixture) | ||
{ | ||
options.quadraticSolver = "sirius"; |
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.
When I look at the fixture (the part that does not "check" anything), I wish it could be turned into an class QuadraticProblem.
If this is possible, in unit tests we could have :
BOOST_FIXTURE_TEST_CASE(simple_qp_one_var, QpFixture)
{
QuadraticProblem QP(/*whatever needed*/);
QP.addVariable("x", 0, 1, -0.5, 1);
QP.solve();
BOOST_CHECK(QP.success());
BOOST_TEST(QP.solution() == {0.25});
....
}
Instead, we have to manipulate problemeAResoudre things, we don't want to read about as a user.
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.
I actually want the checks to be explicitely on problemAResoudre, which holds both the inputs and the outputs of this ortools_quadratic_wrapper
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.
Just some minor remarks, otherwise all good for me !
@@ -69,7 +70,7 @@ toc_depth: 2 | |||
#### Build | |||
* vcpkg (linux, sirius) (#2078) (#2090) (#2145) | |||
* Remove src/antares-deps (#2182) | |||
* Use OR-Tools v9.11-rte1.1 (#2437) | |||
* Use OR-Tools v9.11-rte1.2 (#2574) |
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.
I see that there is or-tools 9.12 now, could we use it ?
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.
we are not yet able to make an RTE release for sirius support (CI issues with Windows)
@@ -17,7 +17,8 @@ hide: | |||
| --adequacy | Force the simulation in [adequacy](04-parameters.md#mode) mode | | |||
| --parallel | Enable [parallel](optional-features/multi-threading.md) computation of MC years | | |||
| --force-parallel=VALUE | Override the max number of years computed [simultaneously](optional-features/multi-threading.md) | | |||
| --solver=VALUE | The optimization solver to use. Possible values are: `sirius` (default), `coin`, `xpress`, `scip` | | |||
| --linear-solver=VALUE | The optimization solver to use for linear problems. Possible values are: `sirius` (default), `coin`, `xpress`, `scip` | |
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.
Breaking change in the API should be expressed as a change in major version number.
@@ -25,7 +25,7 @@ target_link_libraries(${PROJ} | |||
Antares::linear-problem-api | |||
Antares::logs | |||
Antares::solverUtils | |||
ortools::ortools | |||
$<LINK_LIBRARY:WHOLE_ARCHIVE,ortools::ortools> |
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.
Can you add a comment for using this generator expression and not a simple link
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.
And maybe even write an ADR on the subject
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 temporary workaround to a math_opt bug proposed by google. i forgot to link the github discussion, i'll put it in the comments
@@ -54,14 +54,16 @@ void OPT_InitialiserLeSecondMembreDuProblemeLineaire(PROBLEME_HEBDO*, int, int, | |||
void OPT_InitialiserLeSecondMembreDuProblemeQuadratique(PROBLEME_HEBDO*, int); | |||
void OPT_InitialiserLesCoutsLineaire(PROBLEME_HEBDO*, const int, const int); | |||
void OPT_InitialiserLesCoutsQuadratiques(PROBLEME_HEBDO*, int); | |||
bool OPT_AppelDuSolveurQuadratique(PROBLEME_ANTARES_A_RESOUDRE*, const int); | |||
bool OPT_AppelDuSolveurQuadratique(const OptimizationOptions& options, |
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.
Yes : this big header was already there.
What we're doing here is make it grow. We should not.
We should create a opt_appel_solveur_quadratique.h associated to the opt_appel_solveur_quadratique.cpp.
Besides split this opt_fonctions.h would be an easy and not very long work (I think).
It would avoid compilation slowness.
We should remember to clean regularly.
So we should open a ticket for this.
|
||
using namespace Antares; | ||
|
||
bool OPT_AppelDuSolveurQuadratique(PROBLEME_ANTARES_A_RESOUDRE* ProblemeAResoudre, | ||
void SolveWithSirius(const Solver::Optimization::OptimizationOptions& options, |
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.
ok.
Then we could make SolveWithSirius(...)
static (meaning : private to the current source file).
Besides, why do we declare this function and then define it in the same source file ?
We could only define it before the call.
|
This PR adds the possibility for the user to choose quadratic (QP) solvers other than sirius, dor adequacy problems.
It does so by interfacing Antares QP description (PROBLEME_ANTARES_A_RESOUDRE) to the new OR-Tools MathOpt API.
The new "quadratic-solver" parameter allows the user to switch solver:
Breaking change: