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

feat: Adding log parts #3019

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

feat: Adding log parts #3019

wants to merge 246 commits into from

Conversation

arng40
Copy link
Contributor

@arng40 arng40 commented Mar 4, 2024

This PR aim to improve the readibility of the logs by adding log separation the differents parts of the log and
the first step for displaying specific log or timestep we want.

The log separation identified are

  • Input Initialisation : ProblemManager::postInputInitializationRecursive();
  • Mesh generation : ProblemManager::generateMesh();
  • NumericalMethods: ProblemManager::applyNumericalMethods();
  • Mesh data registration : ProblemManager::egisterDataOnMeshRecursive();
  • Group & subgroups initialization functions : ProblemManager::initialize();
  • Each cycle time-step : EventManager::run()
  • Cleanup : basicCleanup()

Example of an input :

######################################################################
##                             TIMESTEP                             ##
######################################################################
##  - Time       : 00h00m00s out of 1d, 03h46m40s (0% completed)    ##
##                 0 s / 100000 s                                   ##
##  - Delta Time : 01h23m20s (5000 s)                               ##
##  - Cycle      : 0                                                ##
##                                                                  ##

    Attempt:  0, ConfigurationIter:  0, NewtonIter:  0
        ( Rflow ) = ( 4.82e-04 )        ( Rwell ) = ( 4.74e-03 )        ( R ) = ( 4.76e-03 )
        Last LinSolve(iter,res) = (   1, 1.64e-11 )
        compositionalMultiphaseFlow: Max pressure change = 19297.019 Pa (before scaling)
        compositionalMultiphaseFlow: Max component density change = 0.346 kg/m3 (before scaling)
        reservoirSystem: Global solution scaling factor = 1
        compositionalMultiphaseFlow: Max phase volume fraction change = 0.0004
[...]
Rank 0: Writing out restart file at ./staircase_co2_wells_3d_restart_000000004/rank_0000000.hdf5

##                             TIMESTEP                             ##
######################################################################

arng40 added 30 commits February 9, 2024 16:11
…feature/dudes/table-layout"

This reverts commit 8f74cfa.
@MelReyCG MelReyCG changed the title Log part added feat: Adding log parts Oct 17, 2024
@arng40 arng40 requested a review from MelReyCG October 17, 2024 12:50
@MelReyCG MelReyCG added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review labels Oct 31, 2024
@@ -48,7 +48,7 @@ class TableLayout
/**
* @brief Enumeration for table sections.
*/
enum Section { header, values };
enum LogPart { header, values };
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you really would like to rename this enum (at least, not to LogPart).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revet

Comment on lines 77 to 87
/**
* @brief Draw the first part of the section. It include the title and optionnaly, the description(s);
* @param os An output stream (by default, std::cout)
*/
void begin( std::ostream & os = std::cout );

/**
* @brief Draw the last part of the section. It include the title
* @param oss An output stream (by default, std::cout)
*/
void end( std::ostream & oss = std::cout ) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, we seem to be able to begin() on multiple output targets. So, to ensure that the content is the same everywhere, I would expect begin() to be const, and internal values like m_sectionWidth to be computed when assessors like addDescription() are used.

Side note: addEndDescription() has no effect on m_sectionWidth, is it the desired behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after addDescription() we call void formatAndInsertDescriptions() where we check m_sectionWidth

void begin( std::ostream & os = std::cout );

/**
* @brief Draw the last part of the section. It include the title
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Draw the last part of the section. It include the title
* @brief Draw the last part of the section. It include the title and optionnaly, the end description(s).

* @brief Set the minimal width of a row
* @param minWidth The minimal width of the table
*/
void setMinWidth( integer const & minWidth );
Copy link
Contributor

Choose a reason for hiding this comment

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

a good feature in the future would be to have a setMaxWidth() with line wrapping, to ensure that a section cannot be too large.

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 could do this in another PR

Comment on lines 126 to 129
/// description border margin
static constexpr integer m_marginBorder = 2;
/// numbers of character used as border
static constexpr integer m_nbBorderChar = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the # character as a centralized constant 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.

y

m_sectionTitle( string( sectionTitle ) ),
m_sectionWidth( sectionTitle.length() )
{
m_footerTitle = GEOS_FMT( "End : {}", m_sectionTitle );
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. If I was you, I would add a updateLayout() private method that takes in parameter every text size like this one, the description lines, the m_sectionTitle, and update m_rowMinWidth whenever a larger size is found. I would prevent any side case to produce oversized lines.
  2. please use the term part as a prefix for your variables (if you really need one), section is now irrelevant.
  3. What do you think of replacing "End : {}" by "End of {}"

string_view name,
std::vector< string > const & values )
{
string const nameFormatted = GEOS_FMT( "- {}: ", string( name ));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it is the responsability of the class user to add those "- " at the start of their texts (imagine if you only have one description).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we have one description we will not have the "- ", it's only for mutiple values in the description

@@ -170,8 +170,9 @@ bool EventManager::run( DomainPartition & domain )
m_dt = dt_global;
#endif
}

outputTime();
LogPart logPart( "TIMESTEP START" );
Copy link
Contributor

Choose a reason for hiding this comment

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

I would name this part "TIMESTEP" or "Timestep no. {}". It is strange to see "End : TIMESTEP START" at its end.

@arng40 arng40 removed ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Nov 6, 2024
@MelReyCG
Copy link
Contributor

MelReyCG commented Nov 7, 2024

Can you share a full log generated after this PR?

Copy link
Contributor

@paveltomin paveltomin left a comment

Choose a reason for hiding this comment

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

just check what happened with schema/docs changes

@arng40 arng40 requested review from MelReyCG December 10, 2024 10:55
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.

4 participants