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.

Sign up for GitHub

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

Jump to bottom

docs: Log level documentation on remaining package #3494

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

docs: Log level documentation on remaining package #3494

wants to merge 35 commits into from

Conversation

arng40
Copy link
Contributor

@arng40 arng40 commented Dec 17, 2024 •
edited
Loading

This PR follow #3230 and aim to document all log level remaining in GEOS

Extends the new log level documentation to all GEOS :

  • Remove old GEOS_LOG_LEVEL macro located in Logger.hpp
  • Centralize the documentation of log level ( aka LogLevelInfos.hpp ) under each package ( folder directly under coreCompenent/ )

@arng40 arng40 marked this pull request as ready for review December 19, 2024 09:27
@arng40 arng40 changed the title Feature/dudes/log level doc package feat: Feature/dudes/log level doc package Dec 19, 2024
@arng40 arng40 self-assigned this Dec 19, 2024
@arng40 arng40 changed the title feat: Feature/dudes/log level doc package feat: Feature/dudes/log level doc on remaining package Dec 19, 2024
@arng40 arng40 changed the title feat: Feature/dudes/log level doc on remaining package doc: Feature/dudes/log level doc on remaining package Dec 19, 2024
@arng40 arng40 requested a review from Guotong-Ren as a code owner December 19, 2024 14:31
@arng40 arng40 requested a review from MelReyCG January 20, 2025 08:19
rrsettgast
rrsettgast approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@rrsettgast rrsettgast left a comment

Choose a reason for hiding this comment

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

@arng40
Just a couple of typos.

Also, could you create a "log level" section in the documentation to give an example of usage for the revised approach to log levels?

src/coreComponents/events/LogLevelsInfo.hpp
Comment on lines +5 to +8
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
* Copyright (c) 2016-2024 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2024 TotalEnergies
* Copyright (c) 2018-2024 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2023-2024 Chevron
* Copyright (c) 2019- GEOS/GEOSX Contributors
* All rights reserved

src/coreComponents/fileIO/LogLevelsInfo.hpp Outdated
Comment on lines 5 to 8
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
* Copyright (c) 2016-2024 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2024 TotalEnergies
* Copyright (c) 2018-2024 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2023-2024 Chevron
* Copyright (c) 2019- GEOS/GEOSX Contributors
* All rights reserved

src/coreComponents/mesh/LogLevelsInfo.hpp Outdated
Comment on lines 5 to 8
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2018-2020 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2020 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2018-2020 TotalEnergies
* Copyright (c) 2019- GEOSX Contributors
* Copyright (c) 2016-2024 Lawrence Livermore National Security LLC
* Copyright (c) 2018-2024 TotalEnergies
* Copyright (c) 2018-2024 The Board of Trustees of the Leland Stanford Junior University
* Copyright (c) 2023-2024 Chevron
* Copyright (c) 2019- GEOS/GEOSX Contributors
* All rights reserved```

CusiniM
CusiniM reviewed Jan 22, 2025
View reviewed changes
src/coreComponents/events/LogLevelsInfo.hpp Show resolved Hide resolved
src/coreComponents/physicsSolvers/LogLevelsInfo.hpp Outdated
Comment on lines 58 to 62
struct Crossflow
{
static constexpr int getMinLogLevel() { return 1; }
static constexpr std::string_view getDescription() { return "Crossflow information"; }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this go in FlowSolvers?

Copy link
Member

Choose a reason for hiding this comment

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

It seems that several subfolder LogLevelsInfo.hpp files were removed and consolidated at a higher level. I am not sure this is what we would want? @arng40 What is the argument for the consolidation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concerning the files that being moved at a higher level, I was divided between keep things simple by putting the log levels at a higher level or group the log levels and set them to correct location.
With your comments I'll go for 2nd option

src/coreComponents/physicsSolvers/LogLevelsInfo.hpp Outdated
Comment on lines 88 to 92
struct DetailedRegionsSourceFluxStats
{
static constexpr int getMinLogLevel() { return 3; }
static constexpr std::string_view getDescription() { return "Print statistics for each source flux in each regions"; }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for this

src/coreComponents/physicsSolvers/LogLevelsInfo.hpp Outdated
Comment on lines 136 to 140
struct RuptureRate
{
static constexpr int getMinLogLevel() { return 3; }
static constexpr std::string_view getDescription() { return "Rupture rate information"; }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

move to SurfaceGenerator?

src/coreComponents/physicsSolvers/LogLevelsInfo.hpp Outdated
};

struct NonlinearSolver
struct StencilConnection
Copy link
Collaborator

Choose a reason for hiding this comment

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

would expect this in Flow too.

src/coreComponents/physicsSolvers/LogLevelsInfo.hpp Outdated
Comment on lines 213 to 225
struct WellComponents
{
static constexpr int getMinLogLevel() { return 1; }
static constexpr std::string_view getDescription() { return "Well components information"; }
};

struct WellControl
{
static constexpr int getMinLogLevel() { return 1; }
static constexpr std::string_view getDescription() { return "Well control information"; }
};

struct WellValidity
Copy link
Collaborator

Choose a reason for hiding this comment

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

and I would expect these to be define in the Well solvers folder.

@arng40 arng40 added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Jan 24, 2025
MelReyCG
MelReyCG requested changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@MelReyCG MelReyCG left a comment

Choose a reason for hiding this comment

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

https://geosx-geosx--3494.com.readthedocs.build/en/3494/docs/sphinx/datastructure/CompleteXMLSchema.html#input-schema-definitions
There still are some undocumented logLevel parameters (like Aquifer). If they do not output any message in the log, they should not appear.

MelReyCG
MelReyCG requested changes Jan 29, 2025
View reviewed changes
Copy link
Contributor

@MelReyCG MelReyCG left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In LogLevelsRegistry.cpp,

  description << "Sets the level of information to write in the standard output (the console typically).\n"
                 "Level 0 outputs no specific information for this solver. Higher levels require more outputs.";

This description can be improved:

  • properly mention that the levels adds up with the lower levels,
  • as it is written for any Group, even those that are unrelated to solvers, you cannot relate to any "solver" concept.

@arng40 arng40 requested a review from dkachuma as a code owner January 30, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CusiniM CusiniM CusiniM left review comments

@rrsettgast rrsettgast rrsettgast approved these changes

@MelReyCG MelReyCG MelReyCG requested changes

@acitrain acitrain acitrain approved these changes

@sframba sframba Awaiting requested review from sframba sframba is a code owner

@castelletto1 castelletto1 Awaiting requested review from castelletto1 castelletto1 is a code owner

@jhuang2601 jhuang2601 Awaiting requested review from jhuang2601 jhuang2601 is a code owner

@paveltomin paveltomin Awaiting requested review from paveltomin paveltomin is a code owner

@frankfeifan frankfeifan Awaiting requested review from frankfeifan frankfeifan is a code owner

@ryar9534 ryar9534 Awaiting requested review from ryar9534 ryar9534 is a code owner

@matteofrigo5 matteofrigo5 Awaiting requested review from matteofrigo5 matteofrigo5 is a code owner

@VidarStiernstrom VidarStiernstrom Awaiting requested review from VidarStiernstrom VidarStiernstrom is a code owner

@wrtobin wrtobin Awaiting requested review from wrtobin wrtobin is a code owner

@untereiner untereiner Awaiting requested review from untereiner untereiner is a code owner

@corbett5 corbett5 Awaiting requested review from corbett5 corbett5 is a code owner

@cssherman cssherman Awaiting requested review from cssherman cssherman is a code owner

@Guotong-Ren Guotong-Ren Awaiting requested review from Guotong-Ren Guotong-Ren is a code owner

@dkachuma dkachuma Awaiting requested review from dkachuma dkachuma is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@arng40 arng40

Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI DO NOT MERGE ! type: documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arng40 @rrsettgast @CusiniM @acitrain @MelReyCG