Skip to content

refactor: cleanup logInfo #3755

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
Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

refactor: cleanup logInfo #3755

wants to merge 21 commits into from
+217 −682

Conversation

paveltomin
Copy link
Contributor

No description provided.

@paveltomin paveltomin requested a review from arng40 August 4, 2025 23:50
@paveltomin paveltomin self-assigned this Aug 4, 2025
@paveltomin paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Aug 4, 2025
@paveltomin paveltomin requested review from victorapm and removed request for arng40 August 4, 2025 23:52
arng40

This comment was marked as duplicate.

Sign in to view

arng40
arng40 reviewed Aug 5, 2025
View reviewed changes
Copy link
Contributor

@arng40 arng40 left a comment

Choose a reason for hiding this comment

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

This PR was clearly needed, thanks !

src/coreComponents/functions/LogLevelsInfo.hpp
{
static constexpr int getMinLogLevel() { return 1; }
static constexpr std::string_view getDescription()
{
return "Output the loaded/computed table data in the log if succinct enough,"
Copy link
Contributor

Choose a reason for hiding this comment

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

why all the sentence has been simplified, it contained useful information for the user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this specific one was simply wrong, no? nothing to do with csv output which is controlled by a different flag

@paveltomin paveltomin added the flag: requires rebaseline Requires rebaseline branch in integratedTests label Aug 5, 2025
@paveltomin
Copy link
Contributor Author

rebaseline is needed
no real diffs: INFO: No unfiltered differences were found.
but diffs like that:

********************************************************************************
Error: /Problem/FieldSpecifications/initialComposition_C10
	Group has a child 'logLevel' in the baseline file but not the file to compare.
********************************************************************************

dkachuma
dkachuma approved these changes Aug 6, 2025
View reviewed changes
@paveltomin paveltomin removed the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Aug 6, 2025
@paveltomin paveltomin requested a review from rasimHZ as a code owner August 12, 2025 17:43
@paveltomin paveltomin requested a review from Copilot August 15, 2025 20:56
Copilot
Copilot AI reviewed Aug 15, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the logging infrastructure throughout the GEOS codebase by consolidating and standardizing logInfo structures across different modules. The primary purpose is to cleanup and simplify the logging system by removing duplicate and redundant log level definitions, reorganizing them into more coherent categories, and establishing a cleaner hierarchy.

  • Consolidates various logInfo structures into more general categories (e.g., LinearSolverConfiguration → LinearSolver, BoundaryCondition → BoundaryConditions)
  • Removes redundant and module-specific log level definitions in favor of centralized ones
  • Updates all logging calls throughout the codebase to use the new consolidated log level names

Reviewed Changes

Copilot reviewed 89 out of 89 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
LogLevelsInfo.hpp files Consolidates and standardizes log level definitions across modules
Physics solver files Updates log level registrations and calls to use new consolidated names
Output files Removes specialized timer categories in favor of centralized approach
Input files Removes explicit logLevel attributes from field specifications
CMakeLists.txt Removes deleted LogLevelsInfo.hpp file references

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

MelReyCG
MelReyCG reviewed Aug 22, 2025
View reviewed changes
src/coreComponents/constitutive/fluid/multifluid/CO2Brine/CO2BrineFluid.cpp Outdated
Comment on lines 344 to 345
!isClone && m_writeCSV,
!isClone && m_writeInLog
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of removing the struct members (as C++17 don't have the named struct initializers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted, i misunderstood the meaning, sorry

src/coreComponents/constitutive/fluid/multifluid/CO2Brine/CO2BrineFluid.hpp
@@ -210,6 +210,9 @@ class CO2BrineFluid : public MultiFluidBase
/// Output csv file containing informations about PVT
integer m_writeCSV;

/// Write PVT tables in log
bool m_writeInLog;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not keeping local?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for symmetry with m_writeCSV but can revert if you prefer

src/coreComponents/mesh/LogLevelsInfo.hpp
{
static constexpr int getMinLogLevel() { return 2; }
static constexpr std::string_view getDescription() { return "well statistics information"; }
static constexpr std::string_view getDescription() { return "Information on fields import"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

did you removed some details here?

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 don't think so, the removed part was inaccurate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MelReyCG MelReyCG MelReyCG left review comments

@arng40 arng40 arng40 left review comments

Copilot code review Copilot Copilot left review comments

@dkachuma dkachuma dkachuma approved these changes

@Guotong-Ren Guotong-Ren Guotong-Ren approved these changes

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

@CusiniM CusiniM Awaiting requested review from CusiniM CusiniM 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

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

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

@tjb-ltk tjb-ltk Awaiting requested review from tjb-ltk tjb-ltk is a code owner

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

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

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

@victorapm victorapm Awaiting requested review from victorapm

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

At least 1 approving review is required to merge this pull request.

Assignees

@paveltomin paveltomin

Labels
flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: cleanup / refactor Non-functional change (NFC)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@paveltomin @dkachuma @MelReyCG @Guotong-Ren @arng40