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

Feature/low level logs #723

Closed

Conversation

a-zakir
Copy link
Contributor

@a-zakir a-zakir commented Nov 25, 2023

No description provided.

const std::string MATHLOGGERCONTEXT = "Benders";

inline std::string Indent(int size) { return std::string(size, ' '); }
const std::string ITE = Indent(10) + "ITE";
Copy link
Contributor

Choose a reason for hiding this comment

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

As we have clean indents we may use ITERATION instead of ITE and adjust indentation

public:
explicit LogDestination(std::ostream* stream) : stream_(stream) {}

// for std::endl
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented code if useless

@@ -153,6 +155,8 @@ void BendersMpi::master_build_cuts(
for (const auto &subproblem_data_map : gathered_subproblem_map) {
for (auto &&[_, subproblem_data] : subproblem_data_map) {
SetSubproblemCost(GetSubproblemCost() + subproblem_data.subproblem_cost);
// compute delta_cut >= options.CUT_MASTER_TOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment does not seem related to what happens here

Copy link
Contributor Author

@a-zakir a-zakir Dec 7, 2023

Choose a reason for hiding this comment

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

yes it's a reminder for a specific data....

src/cpp/benders/benders_mpi/BendersMPI.cpp Show resolved Hide resolved
src/cpp/benders/factories/BendersFactory.cpp Outdated Show resolved Hide resolved
void MathLogger::write_header() {
setHeadersList();
for (const auto& header : Headers()) {
TheLogDestination() << header;
Copy link
Contributor

Choose a reason for hiding this comment

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

LogDestination() instead of TheLogDestination() seems more natural

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've a struct named LogDestination, i suggest LogsDestination

void MathLoggerBase::Print(const CurrentIterationData& data) {
TheLogDestination() << Indent(10) << data.it;
if (data.lb == -1e20)
TheLogDestination() << Indent(20) << "-INF";
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer that we really print 1e+20 and 1e-20 in the math logs as these are the actual values of the data. the purpose of the math logs is to monitor exactly what happens in the data and not to have an interpretation of it as infinity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you say, i do


void MathLoggerBendersByBatch::Print(const CurrentIterationData& data) {
TheLogDestination() << Indent(10) << data.it;
if (data.lb == -1e20)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

void MathLoggerBendersByBatch::setHeadersList() {
MathLogger::setHeadersList({ITE, LB, MINSIMPLEX, MAXSIMPLEX, TIMEMASTER,
SUB_PROBLEMS_TIME_CPU, SUB_PROBLEMS_TIME_WALL,
TIME_NOT_DOING_MASTER_OR_SUB_PROBLEMS_WALL});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add this log to the base case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coming...

TheLogDestination() << Indent(15) << std::setprecision(2)
<< data.subproblems_walltime;

TheLogDestination() << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add missing data

Copy link

watermelon-copilot-for-code-review bot commented Dec 7, 2023

Watermelon AI Summary

AI Summary deactivated by a-zakir

No results found in GitHub PRs :(

No results found in Jira Tickets :(

No results found in Confluence Docs :(

No results found in Slack Threads :(

No results found in Notion Pages :(

No results found in Linear Tickets :(

No results found in Asana Tasks :(

antares-xpansion is an open repo and Watermelon will serve it for free.
🍉🫶
Have you starred Watermelon?

@@ -537,7 +537,7 @@ def update_last_study_with_sensitivity_results(self):

Choose a reason for hiding this comment

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

This PR contains console logs. Please review or remove them.

@@ -17,6 +17,7 @@ int RunBenders(char** argv, const std::filesystem::path& options_file,
const BENDERSMETHOD& method) {

Choose a reason for hiding this comment

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

This PR contains console logs. Please review or remove them.

setHeadersList();
for (const auto& header : Headers()) {
LogsDestination() << header;
}

Choose a reason for hiding this comment

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

This PR contains console logs. Please review or remove them.

@@ -537,7 +537,7 @@ def update_last_study_with_sensitivity_results(self):

def is_antares_study_output(self, study: Path):
_, ext = os.path.splitext(study)
return ext == ".zip" or os.path.isdir(study) and '-Xpansion' not in study.name
return ext == ".zip" or (os.path.isdir(study) and '-Xpansion' in study.name)

Choose a reason for hiding this comment

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

This PR contains console logs. Please review or remove them.

@@ -56,7 +56,7 @@ BENDERS_OPTIONS_MACRO(SOLVER_NAME, std::string, "COIN", asString())
// json file in output/expansion/

Choose a reason for hiding this comment

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

This PR contains console logs. Please review or remove them.

@a-zakir a-zakir mentioned this pull request Dec 12, 2023
@a-zakir a-zakir closed this Dec 12, 2023
a-zakir added a commit that referenced this pull request Jan 4, 2024
see review and history in #723

---------

Co-authored-by: Thomas Bittar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants