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

Adding logLevel documentation #3089

Closed
wants to merge 22 commits into from
Closed

Conversation

arng40
Copy link
Contributor

@arng40 arng40 commented Apr 23, 2024

This PR aim to document all logLevel in GEOS and to invite the contributors to set descriptions when they enable the logLevel wrapper:

Currently, we have this output in all Group documentation files:

========= ========= ======== ================
Name      Type      Default  Description                                                                             
========= ========= ======== ================
[...]
logLevel  integer   0        Log level     
========= ========= ======== ================

This PR brings more context to all logLevels of GEOS:

I.e. SolidMechanics_LagrangianFEM.rst :

========= ========= ======== ========================================================================================
Name      Type      Default  Description                                                                             
========= ========= ======== ========================================================================================
[...]
logLevel  integer   0        | Sets the level of information to write in the standard output (the console typically).
                             | A level of 0 outputs minimal information, higher levels require more.                 
                             | logLevel >= 1                                                                         
                             |  - Information on line search                                                         
                             |  - Information on global solution scaling factor                                      
                             |  - Information on the timestep                                                        
                             |  - Print residual norm                                                                
                             | logLevel >= 1 and configuration didn't converge                                       
                             |  - Information about testing new configuration and print the time step                
                             | logLevel >= 1 and incorrect solution                                                  
                             |  - Information about line search failed                                               
                             | logLevel >= 1 and linear system                                                       
                             |  - Information on number of iterations and residual reduction                         
                             | logLevel >= 1 and non linear system                                                   
                             |  - Information on each newton Iteration                                               
                             | logLevel >= 1 and residual norm above the max allowed residual norm                   
                             |  - Indicate allowed residual norm                                                     
                             | logLevel >= 2                                                                         
                             |  - Output to screen the assembled linear system and solutions (matrices and vectors)  
                             |  - Infos on residuals values                                                          
                             | logLevel >= 2 and target set is empty                                                 
                             |  - Warning about boundary conditions                                                  
                             | logLevel >= 3                                                                         
                             |  - Output to file the assembled linear system and solutions (matrices and vectors)    
========= ========= ======== ========================================================================================

@MelReyCG MelReyCG changed the title Add a way to expend the description of a wrapper Adding logLevel documentation Apr 24, 2024
Comment on lines 94 to 103
try
{
WrapperBase & wrapper = getWrapperBase( logLevelName );
appendLogLevelToWrapper( wrapper, log );
}catch( std::domain_error const & )
{
WrapperBase & wrapper = registerWrapper( logLevelName, &m_logLevel );
enableLogLevelInput();
appendLogLevelToWrapper( wrapper, log );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In this project, try-catch is for adding more info to errors, not to recover from an erroneous state.
Use a if-else syntax.

* @brief Append a log level/description to the description of the wrapped object
* @param log An entry (log level/ log Description) to append
*/
void appendLogLevel( std::pair< const std::string, const std::string > log );
Copy link
Contributor

Choose a reason for hiding this comment

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

appendLogLevelDescription( string_view loggingCondition, string_view caseDescription ) ?

Comment on lines 750 to 752
/// Map for building the log level string for each wrapper
/// key : logLevel, values : description(s) for a level
std::map< std::string, std::vector< std::string > > m_logLevelsDescriptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Only one logLevel per Group = only one map per Group
Every wrapper should not have logLevel functionnalities, Group should.
Those method & attributes you added in Wrapper are not at the right place.

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.

Blocking comments

@arng40 arng40 marked this pull request as ready for review May 27, 2024 07:42
@arng40 arng40 requested review from ryar9534 and MelReyCG May 27, 2024 07:45
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 79.50311% with 33 lines in your changes missing coverage. Please review.

Project coverage is 55.75%. Comparing base (3bf12d2) to head (b47ffe8).
Report is 8 commits behind head on develop.

Current head b47ffe8 differs from pull request most recent head c56f007

Please upload reports for the commit c56f007 to get more accurate results.

Files Patch % Lines
...sicsSolvers/surfaceGeneration/SurfaceGenerator.cpp 0.00% 6 Missing ⚠️
...hysicsSolvers/multiphysics/HydrofractureSolver.cpp 0.00% 4 Missing ⚠️
...s/physicsSolvers/simplePDE/PhaseFieldDamageFEM.cpp 0.00% 3 Missing ⚠️
...onstitutive/relativePermeability/RelpermDriver.cpp 0.00% 2 Missing ⚠️
...olvers/solidMechanics/SolidMechanicsStateReset.cpp 0.00% 2 Missing ⚠️
...ers/surfaceGeneration/EmbeddedSurfaceGenerator.cpp 0.00% 2 Missing ⚠️
src/coreComponents/fileIO/Outputs/ChomboIO.cpp 0.00% 1 Missing ⚠️
src/coreComponents/fileIO/Outputs/VTKOutput.cpp 0.00% 1 Missing ⚠️
...onents/linearAlgebra/interfaces/hypre/HypreMGR.cpp 0.00% 1 Missing ⚠️
...mponents/mesh/simpleGeometricObjects/Rectangle.cpp 0.00% 1 Missing ⚠️
... and 10 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3089      +/-   ##
===========================================
+ Coverage    55.71%   55.75%   +0.03%     
===========================================
  Files         1031     1031              
  Lines        87698    87823     +125     
===========================================
+ Hits         48863    48962      +99     
- Misses       38835    38861      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paveltomin
Copy link
Contributor

How is all that connects with proposal about logLevel labels in #3014 ?

@arng40
Copy link
Contributor Author

arng40 commented Jun 20, 2024

This PR is related to the #3179 and not the #3014.

Copy link
Contributor

@TotoGaz TotoGaz left a comment

Choose a reason for hiding this comment

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

Could we discuss the this in a meeting first?

* @param levelCondition The level condition to append
* @param logDescription The log description to append
*/
void appendLogLevelDescription( string_view levelCondition, string_view logDescription );
Copy link
Contributor

@TotoGaz TotoGaz Jun 20, 2024

Choose a reason for hiding this comment

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

Is there a way, instead of having now 4 public member functions and 2 attributes directly related to logs (and a entry in the viewKeysStruct) to gather all the behaviors related to the log into a dedicated class?
I personally do not want to add more and more stuff into Group which is already big enough (+/- 120 public member functions IIRC).

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit off-topic comment : I don't know if it would help, but in a long-term effort. I would say that if we use a similar approach with the group's wrappers with a WrapperContainers, we could remove around 36 members (including const/non-const), allowing to clarify that methods like "size()" are related to Wrappers.

@arng40
Copy link
Contributor Author

arng40 commented Jun 24, 2024

@TotoGaz Thanks for your feedback, in reply to your suggestion :
Would a highly coupled class be an issue? Group would only have 1 attribute & 1 getter for this class.
Or is it better to have a purely not coupled class? Group would have 1 attribute, 1 getter, and 1 method (appendLogLevelDescription()).

@MelReyCG MelReyCG closed this Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review all log level Wrapper documentation (Wrapper::appendDescription()),
4 participants