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

8.8 solver parameters [ANT-2280] #2466

Merged
merged 12 commits into from
Nov 8, 2024
Merged

Conversation

payetvin
Copy link
Contributor

@payetvin payetvin commented Oct 17, 2024

This pull request introduces several significant changes to the Antares project, focusing on adding support for solver-specific parameters and restructuring the optimization options. The key changes include the creation of a new optimization-options module, refactoring of existing code to use this module, and enhancements to error handling related to solver parameters.

New Module Addition:

  • Added a new optimization-options module to encapsulate all options related to optimization (src/libs/antares/optimization-options/*). [1] [2]

Code Refactoring:

  • Refactored StudyLoadOptions and Parameters classes to use the new OptimizationOptions structure for managing solver-related options (src/libs/antares/study/load-options.h, src/libs/antares/study/load-options.cpp, src/libs/antares/study/parameters.h, src/libs/antares/study/parameters.cpp). [1] [2] [3] [4]

Error Handling Enhancements:

  • Introduced a new InvalidSolverSpecificParameters exception to handle invalid solver-specific parameters (src/libs/antares/exception/LoadingError.hpp, src/libs/antares/exception/LoadingError.cpp). [1] [2]

Documentation Updates:

  • Updated the command line reference guide to include the new --solver-parameters option (docs/reference-guide/10-command_line.md).

Miscellaneous:

  • Moved the add_subdirectory(writer) command in CMakeLists.txt to ensure proper build order (src/libs/antares/CMakeLists.txt). [1] [2]

@payetvin payetvin changed the title 8.8 solver parameters 8.8 solver parameters [ANT-2280] Oct 28, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use the same exhaustive example to show both scip & xpress usages : Set solver-specific parameters, for instance --solver-parameters="THREADS 1 PRESOLVE 1" for XPRESS or --solver-parameters="parallel/maxnthreads 1, lp/presolving TRUE" for SCIP. Syntax is solver-dependent, and only supported for SCIP & XPRESS.

@@ -11,7 +11,7 @@
#include <antares/version.h>
#include <antares/writer/writer_factory.h>

#include "signal-handling/public.h"
#include "../signal-handling/public.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, why do we have this new magic inclusion, though no change here seems to call for this inclusion ?

src/solver/misc/options.cpp Outdated Show resolved Hide resolved
src/solver/misc/options.cpp Outdated Show resolved Hide resolved
@@ -6,7 +6,7 @@
#include <boost/test/unit_test.hpp>

#include <timeseries-numbers.h>
#include "ts-generator/generator.h"
#include "../../../../solver/ts-generator/generator.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

@pet-mit pet-mit left a comment

Choose a reason for hiding this comment

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

update antares batch launcher script

@pull-request-size pull-request-size bot added size/XL and removed size/L labels Nov 7, 2024
Copy link
Contributor

@pet-mit pet-mit left a comment

Choose a reason for hiding this comment

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

would like to have someone else's opinion on batchrun. rest is OK for me

@@ -19,6 +18,9 @@ add_subdirectory(io)
add_subdirectory(exception)

add_subdirectory(sys)
add_subdirectory(writer)

add_subdirectory(optimization-options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we add a directory src/libs/antares/optimization-options ?
We could add a folder src/solver/optimization-options instead, it would seem more natural, wouldn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parameters are related to study

@@ -24,12 +24,17 @@
**
** SPDX-License-Identifier: licenceRef-GPL3_WITH_RTE-Exceptions
*/
#include "antares/optimization-options/options.h"
Copy link
Contributor

@guilpier-code guilpier-code Nov 8, 2024

Choose a reason for hiding this comment

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

Current header checkLoadedInputData.h doesn't seem to use types defined in antares/optimization-options/options.h.
The latter header should not be included here.

src/libs/antares/exception/LoadingError.cpp Show resolved Hide resolved
src/solver/misc/options.cpp Show resolved Hide resolved
src/solver/misc/options.cpp Show resolved Hide resolved
@@ -246,19 +127,13 @@ int main(int argc, char* argv[])
if (not finder.list.empty())
Copy link
Contributor

@guilpier-code guilpier-code Nov 8, 2024

Choose a reason for hiding this comment

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

About :

if (not finder.list.empty())

is useless : the loop over the found studies won't start if there are no study in the finder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but there will be no clear error message without this condition

src/tools/batchrun/main.cpp Show resolved Hide resolved

// argv[0] => executable name
// argv[1] => directory name
for (int ii = 2; ii < argc; ii++)
Copy link
Contributor

Choose a reason for hiding this comment

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

-i / --input is no longer available as a batchrun command line option.
Is this intended ?

We now imply that the search directory is necessarily the first argument the user will give, and without the -i introduction.
I'm not sure of that.

I'm tempted to suggest the following command line pattern :
./batchrun -i <search dir> <the other args>, if it's doable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed it only works if the study is given as the first arg
same behavior as develop, maybe open a ticket about it ?

Copy link

sonarqubecloud bot commented Nov 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@payetvin payetvin merged commit 2b0ff9b into release/8.8.x Nov 8, 2024
6 of 7 checks passed
@payetvin payetvin deleted the feature/8.8-solver-parameters branch November 8, 2024 16:37
flomnes pushed a commit that referenced this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants