Skip to content

feat: Add the table error managment #3614

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

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

Conversation

arng40
Copy link
Contributor

@arng40 arng40 commented Mar 28, 2025

This PR goes after #3537

The main goal of this PR is to not stop the simulation when an error appear during the lifetime of the table.

  • Working also for the csv generation with the table.
  • All the errors will be append to the table and will give an indication of the table's consistency.

Exemple with a table from SinglePhaseStatistics.cpp

----------------------------------------------------------------------------------------------------------
|                           SinglePhaseFlowStatistics, Channel (time 20000 s):                           |
|--------------------------------------------------------------------------------------------------------|
|             statistics             |         min          |       average        |         max         |
|------------------------------------|----------------------|----------------------|---------------------|
|                      Pressure[Pa]  |                 inf  |  512792.08446348715  |  6003671.560233745  |
|               Delta pressure [Pa]  |  -4367397.539716403  |                   /  |  6003671.560233745  |
|                   Temperature [K]  |                   0  |                   0  |                  0  |
|------------------------------------|-------------------------------------------------------------------|
|                        statistics  |                                                            value  |
|------------------------------------|-------------------------------------------------------------------|
|  Total dynamic pore volume [rm^3]  |                                                                   |
|------------------------------------|-------------------------------------------------------------------|
|             Total fluid mass [kg]  |                                                50038.50017705067  |
|--------------------------------------------------------------------------------------------------------|
|  Error : One or more data lines are not equal to the number of headers                                 |
|  Data can be missing/misaligned                                                                        |
|  Warning : Invalid values detected (nan/inf).                                                          |
----------------------------------------------------------------------------------------------------------

- uniformizing behaviour of TableFunction regarding other outputing components (OutputBase...)
…/row by using standard variables) + possibility to change column/row major convention for table2Ds
arng40 added 7 commits April 17, 2025 17:43
commit 5537695
Merge: ecd5f7f 4aa2c9f
Author: MelReyCG <[email protected]>
Date:   Thu Apr 17 13:45:39 2025 +0200

    Merge branch 'develop' into feature/rey/umpire-csv-stats

commit ecd5f7f
Author: MelReyCG <[email protected]>
Date:   Thu Apr 17 11:31:26 2025 +0200

    🦺 rebaseline for "parallelThread" moving

commit 4aa2c9f
Author: Randolph Settgast <[email protected]>
Date:   Wed Apr 16 13:33:48 2025 -0700

    fix: remove incorrect use of template keyword in function calls with no template argument list (#3625)

    * in cases where there is a templated base class and a templated function func<T>(T t), calls such as derived::template func( t ) is actually non-compliant. It is replaced with derived::template func<>( T )

commit 1b7ebdc
Merge: 6e4ec16 696e599
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 17:37:16 2025 +0200

    Merge branch 'develop' into feature/rey/umpire-csv-stats

commit 6e4ec16
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 16:48:37 2025 +0200

    ♻️ simplifying isNewlyOpened in header

commit 6cb75f1
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 16:46:17 2025 +0200

    ♻️ simplifying isNewlyOpened in header

commit bc39174
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 16:45:05 2025 +0200

    ♻️ simplifying isNewlyOpened because behaviour was potencially not accurate in some situations + was complexifying the code

commit 9b46fe9
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 16:43:44 2025 +0200

    💡 added todo for error listing PR 3614

commit 9d2e191
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 16:18:18 2025 +0200

    🐛 💄 update CSV & Log table layouts for parsing & readability + fix percentage computation

commit acb6d27
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 15:00:23 2025 +0200

    ⚰️ dead code

commit 3189e9f
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 14:21:50 2025 +0200

    💄 thinner table layout (table was large)

commit 79ba4cf
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 14:13:28 2025 +0200

    🐛 📝 docs bugfix, 2nd attempt

commit 7b271c5
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 14:06:34 2025 +0200

    🐛 📝 docs bugfix

commit b9f38f9
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 10:44:20 2025 +0200

    📝 docs fixes

commit 7c92938
Author: MelReyCG <[email protected]>
Date:   Wed Apr 16 10:17:56 2025 +0200

    📝 docs fix

commit 696e599
Author: Pavel Tomin <[email protected]>
Date:   Tue Apr 15 10:44:03 2025 -0700

    fix: time step logic for sequential (#3624)

commit 4778151
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 17:45:13 2025 +0200

    🐛 add include

commit b157672
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 17:43:59 2025 +0200

    🩹 better strategy for defining last report cycle & time

commit 479230f
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 17:12:19 2025 +0200

    🎨 uncrustify

commit e36337e
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 17:06:19 2025 +0200

    🐛 correct last timestep writing

commit f547b76
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 16:33:30 2025 +0200

    🐛 wrong csv character (added a global definition of the standard CSV separator)

commit c23c214
Author: Arnaud DUDES <[email protected]>
Date:   Tue Apr 15 16:12:53 2025 +0200

    fix: Log levels generation on documentation generation (#3615)

commit b2d65ce
Author: Jian Huang <[email protected]>
Date:   Tue Apr 15 09:12:18 2025 -0500

    fix: update TFrac tutorial example (#3595)

commit f6b3d86
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 15:11:50 2025 +0200

    🐛 Solve singleton issues

commit 145676e
Author: MelReyCG <[email protected]>
Date:   Tue Apr 15 10:36:19 2025 +0200

    ✅ Added unit-test for MemoryStats & MemoryStatsOutput

commit d0e5b2a
Author: Arnaud DUDES <[email protected]>
Date:   Tue Apr 15 00:49:34 2025 +0200

    refactor: Refactoring raw logs to tables (#3559)

    * solubility table

    * fields table

    * load balancing

    * set mesh partitioner smaller and removed some info if rank size = 1

    * improve CO2 solubility

commit 2404873
Author: Arnaud DUDES <[email protected]>
Date:   Tue Apr 15 00:45:04 2025 +0200

    refactor: FieldSpecification context when targeting wrong fieldName (#3508)

    * add specific fieldName when enter wrong name

    * remove char const and set pstringstream

    * attempt n°3 to output fieldName error

commit 10f0a21
Author: Brian Han <[email protected]>
Date:   Mon Apr 14 12:45:56 2025 -0700

    build: Update RAJA suite to v2025.03.0 (#3613)

    * update tag

    * Update geos spack-generated host-configs

    * Update LvArray submodule

commit 679b39c
Author: MelReyCG <[email protected]>
Date:   Mon Apr 14 09:46:30 2025 +0200

    ⚰️ removed dead attribute "parallelThreads" from exemples

commit 651a6c7
Author: MelReyCG <[email protected]>
Date:   Fri Apr 11 14:07:14 2025 +0200

    📦 the schema

commit 8adab2a
Author: MelReyCG <[email protected]>
Date:   Fri Apr 11 14:07:07 2025 +0200

    🐛 ♻️ used unique ptrs to build the formatters

commit e9e6d7d
Author: MelReyCG <[email protected]>
Date:   Thu Apr 10 18:14:23 2025 +0200

    ✨ adding multiple memory output possibility during simulation
    todo : call setCurrentCycle() for the ultimate call

commit 9446be7
Author: MelReyCG <[email protected]>
Date:   Thu Apr 10 16:58:46 2025 +0200

    🎨 🐛 finished implementation of MemoryStatsOutput

commit 09973c3
Author: MelReyCG <[email protected]>
Date:   Thu Apr 10 11:04:24 2025 +0200

    🐛 adapting header usage

commit 5656c1a
Author: MelReyCG <[email protected]>
Date:   Thu Apr 10 09:50:55 2025 +0200

    🐛 minor refactor

commit ae83b90
Author: MelReyCG <[email protected]>
Date:   Thu Apr 10 09:28:25 2025 +0200

    🐛 wrong include

commit e8ca7b5
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:35:14 2025 +0200

    ✨ Adding MemoryStatsOutput to CMakeLists

commit 0e5758c
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:34:41 2025 +0200

    🐛 various fixes

commit acea035
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:29:19 2025 +0200

    🐛 please don't look

commit f5d5763
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:25:29 2025 +0200

    🐛 constness

commit 4b5efc2
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:22:03 2025 +0200

    🐛 messed some parameters

commit 7a00b83
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 17:10:51 2025 +0200

    🐛 forgot some GEOS_FMT

commit 2434f0f
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 16:55:55 2025 +0200

    🐛 adding specification of isNewlyOpened

commit 2a2e40f
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 16:32:55 2025 +0200

    ♻️ moved implementation of "parallelThread" code from OutputBase to SiloOutput as it is not useful on any other sub-class

commit 1c68d5c
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 16:12:02 2025 +0200

    ♻️ moved "parallelThread" code from OutputBase to SiloOutput as it is not useful on any other sub-class

commit cf3cf92
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 16:07:51 2025 +0200

    ✨ Adding "MemoryStats" to add to "Outputs" to control the umpire stats output

commit ace69ef
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 15:57:32 2025 +0200

    ♻️ minor refactor

commit 143eb00
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 15:37:53 2025 +0200

    🗑️ TODO, remove inherited SILO code

commit 634bc0b
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 15:36:59 2025 +0200

    🐛 wrong accessors for MemoryLogging

commit 9e8ebba
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 15:10:06 2025 +0200

    📦 P4 down so let's try to write the desired schema by hand?

commit 9ffa983
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 14:44:12 2025 +0200

    ✨ adding error aware stream-to-file functions for ouputing CSV memory stats tables

commit 7e7463a
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 14:42:39 2025 +0200

    🎨 minor changes

commit 8b85058
Author: MelReyCG <[email protected]>
Date:   Wed Apr 9 10:38:00 2025 +0200

    ✨ Added a MemoryLoging singleton component to manage the output of memory stats (umpire) with more flexibility

commit e09f499
Author: Matteo Cusini <[email protected]>
Date:   Mon Apr 7 11:03:53 2025 -0700

    fix: Fix runtime cuda error. (#3617)

    * fix: Fix runtime cuda error.

    * fix another similar error.

    * Update src/coreComponents/fieldSpecification/TractionBoundaryCondition.cpp

commit 2e206c1
Author: Ben Corbett <[email protected]>
Date:   Sat Apr 5 11:28:59 2025 -0700

    fix: Fixed leaks in ArrayOfArrays and MeshLevel. (#3602)

    * Fixed leaks in ArrayOfArrays and MeshLevel.

    * submodule update

    * Updated LvArray

    ---------

    Co-authored-by: Matteo Cusini <[email protected]>
Melcx

This comment was marked as duplicate.

@GEOS-DEV GEOS-DEV deleted a comment from Melcx Apr 22, 2025
@GEOS-DEV GEOS-DEV deleted a comment from Melcx Apr 22, 2025
@GEOS-DEV GEOS-DEV deleted a comment from Melcx Apr 22, 2025
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.

Review of the bottom of changes.

"| Temperature = 2 | 17 | 13 | 14 | 15 |\n"
"| Temperature = 3 | 23 | 19 | 20 | 21 |\n"
"|---------------------------------------------------------------------------------------|\n"
"| Warning : The number of values is insufficient for the number of columns * rows: |\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the message here is not "Too much data for ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

{
TableData2D tableData2D;

// 1. Prepare values
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid numbering comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment on lines 110 to 121
if( nRow * nCol > values.size())
{
m_errors->addError( GEOS_FMT( "Warning : The number of values is insufficient for the number of columns * rows:\n"
"Expected {} values, Found {} values", nRow * nCol, values.size() ) );
wellConstructValues.resizeDefault( nRow * nCol, 0 );
}
else
{
m_errors->addError( GEOS_FMT( "Warning : Too much data for the number of columns * rows:\n"
"Expected {} values, Found {} values\n"
"Data may be missaligned", nRow * nCol, values.size() ) );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion of message here:

Suggested change
if( nRow * nCol > values.size())
{
m_errors->addError( GEOS_FMT( "Warning : The number of values is insufficient for the number of columns * rows:\n"
"Expected {} values, Found {} values", nRow * nCol, values.size() ) );
wellConstructValues.resizeDefault( nRow * nCol, 0 );
}
else
{
m_errors->addError( GEOS_FMT( "Warning : Too much data for the number of columns * rows:\n"
"Expected {} values, Found {} values\n"
"Data may be missaligned", nRow * nCol, values.size() ) );
}
if( values.size() < nRow * nCol )
{
m_errors->addError( GEOS_FMT( "Warning: Not enough for the number of columns & rows:\n"
" - Expected {} values ({} columns x {} rows),\n - Found {} values",
nRow * nCol, nCol, nRow, values.size() ) );
wellConstructValues.resizeDefault( nRow * nCol, 0 );
}
else
{
m_errors->addError( GEOS_FMT( "Warning: Too much data for the number of columns & rows:\n"
" - Expected {} values ({} columns x {} rows),\n - Found {} values",
"Data may be missaligned",
nRow * nCol, nCol, nRow, values.size() ) );
}


bool TableData::operator<(TableData const & other) const
{
return m_rows < other.m_rows;
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

// TODO: 1. restore the table non-blocking error system. 2. add this assert 3. add any other error to it.
GEOS_ASSERT( nRow * nCol == values.size() );

array1d< real64 > wellConstructValues( values.size() );
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
array1d< real64 > wellConstructValues( values.size() );
array1d< real64 > wellConstructedValues( values.size() );

or

Suggested change
array1d< real64 > wellConstructValues( values.size() );
array1d< real64 > wellFormedValues( values.size() );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wellFormedValues

std::vector< string > errorText;

/**
* @brief Add an error that will be display at the end of the table
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 Add an error that will be display at the end of the table
* @brief Add an error that will be displayed at the end of the table

Comment on lines 45 to 47
/// View on cell error grouping the cell information to display at the end of the table
/// Contain all the errors
std::vector< string > errorText;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why do you qualify it of a view? It seems more like the "real" list of the error messages (exemples of views: string_view, pointer, list of string_view)
  • I think names like errorMessages/errorsList would emphasize the possibility to have more than one error stored (it is not a single message with multiple lines). The comment also means that only one error can be stored.
  • If the TableErrorListing is responsible of the list managment, why is this std::vector public?
    To keep the possibility of exposing the messages, you could have.
Suggested change
/// View on cell error grouping the cell information to display at the end of the table
/// Contain all the errors
std::vector< string > errorText;
public:
using Iterator = std::vector< string >::const_iterator;
Iterator begin()
{ return errorText.begin(); }
Iterator end()
{ return errorText.end(); }
private:
std::vector< string > errorText;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

errorText should be private indeed, I will fix that

Comment on lines 526 to 528
errorCellsLayout[errorCellsLayout.size() - 1].cells[nbCells - 1].setWidth( 0 );
errorCellsLayout[errorCellsLayout.size() - 1].cells[nbCells - 1].m_cellType = CellType::Value;
errorCellsLayout[errorCellsLayout.size() - 1].cells[nbCells - 1].getLines().emplace_back( error );
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for not using the prepareLayout() method? This code add the need for the non-const CellLayout::getLines(), which is dangerous.
Also, you can use std::vector<T>::last() if you want to (I saw you using front()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

Comment on lines 472 to 473
std::vector< TableLayout::CellLayout >( nbColumns, TableLayout::CellLayout() ), // cells
1 // sublinesCount
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 understand why you set 1 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.

reset to 0

Comment on lines 315 to 329

if( column.isVisible() )
{
if( columns[idxColumn].isVisible())
m_columnLayersCount = std::max( m_columnLayersCount, level + 1 );

if( !column.hasChild() )
Column & column = columns[idxColumn];
CellType cellType = column.m_header.m_layout.m_cellType;

if( !column.hasChild() )
{
++m_totalLowermostColumnCount;
if( cellType!= CellType::Hidden )
{
++m_lowermostColumnCount;
++m_visibleLowermostColumnCount;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a bugfix? Is it linked to the 0->1 change? (I don't see a commit message that talk about a bug or something, but the logic seems quite different)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update and simplify

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 revert it for znd populateData & error ( the 0->1) , the refacto was a bit too much :)

@arng40 arng40 requested a review from MelReyCG April 25, 2025 12:34
@arng40
Copy link
Contributor Author

arng40 commented Apr 30, 2025

I have a bug related to arrayofarray.
@wrtobin has already resolved this bug in this pending PR:
#3223 (comment)
@rrsettgast @CusiniM @wrtobin @corbett5
Could you apply this fix in LvArray

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: no rebaseline Does not require rebaseline type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants