feat: Table subs division #3353

arng40 · 2024-09-13T14:53:12Z

This PR allows the table to be subdivided, but also simplifies the declaration of the table.
Column declarations can be mixed with string and ColumnParam struct, I.E:

TableLayout const layoutTest( "Rank",
                             TableLayout::ColumnParam{"Nodes", TableLayout::Alignment::center},
                             "Edge",
                             TableLayout::ColumnParam{"Faces", TableLayout::Alignment::center},
                             TableLayout::ColumnParam{"Elems", TableLayout::Alignment::center} );

For an example with sub-columns, this will give the following result

TableLayout const layoutTest( "Cras egestas ipsum a nisl. Vivamus variu dolor utsisicdis parturient montes, nascetur ridiculus mus. Duis nascetur ridiculus mus ",
                             { "Rank",
                                TableLayout::ColumnParam{"Nodes", TableLayout::Alignment::center, true, {"local", "ghost"}},
                                "Edge",
                                TableLayout::ColumnParam{"Faces", {"local", "ghost"}},
                                TableLayout::ColumnParam{"Elems", TableLayout::Alignment::center, true, {"local", "ghost"}} 
   			      });

[data decleration]

Output :

               "--------------------------------------------------------------------------------------------------------------------------------------"
               "|  Cras egestas ipsum a nisl. Vivamus variu dolor utsisicdis parturient montes, nascetur ridiculus mus. Duis nascetur ridiculus mus  |"
               "--------------------------------------------------------------------------------------------------------------------------------------"
               "|                         Rank  |           Nodes           |      Edge      |           Faces           |           Elems           |"
               "--------------------------------------------------------------------------------------------------------------------------------------"
               "|                               |    local    |    ghost    |                |    local    |    ghost    |    local    |    ghost    |"
               "--------------------------------------------------------------------------------------------------------------------------------------"
               "|             min(local/total)  |      1      |      2      |       3        |      4      |      5      |      6      |      7      |"
               "|             min(local/total)  |      1      |      2      |       3        |      4      |      5      |      6      |      7      |"
               "--------------------------------------------------------------------------------------------------------------------------------------"

Also, column titles are now centered by default, regardless on the content alignement.

…e-layout

…dudes/table-layout

…feature/dudes/table-layout" This reverts commit 8f74cfa.

…nto feature/dudes/table-layout

… pvt-logs-cp

…e-layout

…e-subs-division

MelReyCG

First pass on test & general functionnalities

src/coreComponents/common/format/table/unitTests/testTable.cpp

src/coreComponents/common/format/table/TableLayout.hpp

MelReyCG · 2024-10-29T09:41:00Z

src/coreComponents/common/format/table/unitTests/testTable.cpp

+                            TableLayout::Column{{"CoordZ"}, TableLayout::Alignment::left},
+                            TableLayout::Column{{"Prev\nelement"}, TableLayout::Alignment::right},
+                            TableLayout::Column{{"Next\nelement"}, TableLayout::Alignment::right}} );
+  tableLayout.setValuesAlignment( TableLayout::Alignment::right );


Is the goal of this test to consolidate the setValuesAlignment() code?
If so,

keep tableLayout non-const,

use your builder pattern here,

no need to test that more that one time, it makes test heavier for no purposes.

setValuesAlignment is for cellValues and alignement TableLayout::Column{} is for header alignment

Ok I see, this is not what we discussed at first, but it is good to be able to modify both setting by this way.
So two requests:

Keep in mind that the value alignement is more likely to be customised than the header alignement. So please modify the valueAlignment in the Column constructors.

Test the setValuesAlignment() only one time.

use your builder pattern here, and declare tableLayout constant.

MelReyCG · 2024-10-29T10:03:37Z

src/coreComponents/common/format/table/unitTests/testTable.cpp

+TEST( testTable, testLineWrap )
 {
-  ////////////
-  //////// If setMargin used elsewhere make it public and remove comments for this test
-  ////////////
-  //test with tiny margin
-  // {
-  //   TableLayout tableLayout( {
-  //     TableLayout::ColumnParam{{"Colonne 1"}, TableLayout::Alignment::center},
-  //     TableLayout::ColumnParam{{"Colonne 2"}, TableLayout::Alignment::center},
-  //     TableLayout::ColumnParam{{"Colonne 3"}, TableLayout::Alignment::right},
-  //     TableLayout::ColumnParam{{"Colonne 4"}, TableLayout::Alignment::right},
-  //     TableLayout::ColumnParam{{"Prev\nelement"}, TableLayout::Alignment::right},
-  //     TableLayout::ColumnParam{{"Next\nelement"}, TableLayout::Alignment::right},
-  //   }, "InternalWellGenerator well_injector1" );
-
-  //   //tableLayout.setMargin( TableLayout::MarginValue::tiny );
-
-  //   TableData tableData;
-  //   tableData.addRow( "value 1", "long value 1", "3.0034", 3.0129877, "" , 1 );
-  //   tableData.addRow(  "value 1", "long value 2", "100.45", 4.0129877, 1 , 2 );
-
-  //   TableTextFormatter const tableText( tableLayout );
-  //   EXPECT_EQ( tableText.toString( tableData ),
-// "\n------------------------------------------------------------\n"
-// "|           InternalWellGenerator well_injector1           |\n"
-// "------------------------------------------------------------\n"
-// "|Colonne 1| Colonne 2  |Colonne 3|Colonne 4|   Prev|   Next|\n"
-// "|         |            |         |         |element|element|\n"
-// "------------------------------------------------------------\n"
-// "| value 1 |long value 1|   3.0034|3.0129877|       |      1|\n"
-// "| value 1 |long value 2|   100.45|4.0129877|      1|      2|\n"
-// "------------------------------------------------------------\n\n"
-//              );
-// }
+  TableLayout tableLayout( {"Cras egestas", "CoordX", "C", "CoordZ", "Prev\nelement", "Next\nelement"} );
+  tableLayout.setTitle( "title" ).setMargin( TableLayout::MarginValue::tiny ).disableLineWrap();
+
+  TableData tableData;
+  tableData.addRow( "1", "2", "3.0", 3.0129877, 2.0f, 1 );
+  tableData.addRow( "1", "2", "3.0", 3.0129877, 2.0f, 1 );


Good thing that we can use a line break in titles :)

testLineBreak would be more accurate as a name (line wrapping term is more dedicated for automatic line breaks).

I really think it should support line breaks within content lines:

tableData.addRow( "1", "2", "one line text", 3.0129877, 2.0f, 1 ); tableData.addRow( "1", "2", "three\nlines\ntext", 3.0129877, 2.0f, 1 ); tableData.addRow( "1", "2", "one line text", 3.0129877, 2.0f, 1 );

I can make line break for cell values if needed, not sure if right this is mandatory

I encountered a scenario that needed this, but I forgot it :(
We can get back on this later.

Ok, what do you think of this?

------------------------------------- | stat name | component | value | ------------------------------------- | pressure | all | 123.456 | | temperature | all | 123.456 | ------------------------------------- | | oil | 123.456 | | mass | gaz | 123.456 | | | water | 123.456 | -------------------------------------

I think it would need to be able to have a TableData::addSeparatorRow(), and we may need multiline for doing a tableData.addRow("mass","oil\ngaz\nwater","123.456\n123.456\n123.456")

MelReyCG

2nd pass, format/table classes

src/coreComponents/common/format/table/TableFormatter.cpp

src/coreComponents/common/format/table/TableLayout.hpp

src/coreComponents/common/format/table/TableFormatter.hpp

MelReyCG · 2024-10-29T14:05:58Z

src/coreComponents/common/format/table/TableLayout.hpp

    /// Vector containing substring column name delimited by "\n"
-    std::vector< string > splitColumnNameLines;
+    std::vector< string > splitColumnNames = {""};


Can a user mess with this parameter? It seems like it should be private.

this parameter is assigned in tableFormatter function

I think it should be annotated as an "internal computed value", as it is not dedicated to be modified (but public, in a set-up structure).

Please, either set it private or anotate it as an "internal computed value".

src/coreComponents/common/format/table/TableFormatter.cpp

MelReyCG · 2024-10-29T14:44:25Z

src/coreComponents/common/format/table/TableFormatter.cpp

+void TableTextFormatter::splitAndMergeColumnHeaders( std::vector< TableLayout::TableColumnData > & tableColumnsData,
+                                                     size_t & nbHeaderRows,
+                                                     std::vector< std::vector< string > > & splitHeaders ) const


Do you really use all those out-parameters?

Why nbHeaderRows does not take into account sub-columns? Can it cause issues? (is it tested?)

I removed nbHeaderRows parameters in all signature, no issue for we add '\n' to subcolumn bc it's not implemented yet, it will just take the first part of the string

I think it needs more work to either fully implement what those lines tries to do:

if( !tableColumnData.subColumn.empty()) { std::vector< std::vector< string > > splitSubColHeaders; size_t nbHeaderSubColRows = 0; splitAndMergeColumnHeaders( tableColumnData.subColumn, nbHeaderSubColRows, splitSubColHeaders ); }

You seem to now correctly store the results of splitAndMergeColumnHeaders() in splitColumnNames.
So what is the use of allDividedHeaderParts out-parameter?

src/coreComponents/common/format/table/TableFormatter.cpp

MelReyCG · 2024-10-29T15:05:29Z

I must say that it is a good thing that your worked on renaming/refactoring some parts, it will helps a lot in understanding.

MelReyCG

I added some requests.

…e-subs-division

MelReyCG · 2024-12-23T10:12:24Z

Work merged in #3406

arng40 added 30 commits February 13, 2024 14:13

Merge remote-tracking branch 'origin/develop' into feature/dudes/tabl…

59a9bc8

…e-layout

add log to wellGeneratorBase

677fdb6

uncrustify + missing doc

623b6ad

first review with correction

37dc5dd

constness modification, doc/test updated

8678fc0

const declaration updated

9475286

doc + minor restructuring

1320651

doc + method for row from vector

f78bd83

clean code + doc + add test for rows from vector

294f075

doc updated

36dd7a4

Merge commit 'c11ad8ecd2d0d0aba5cd0661eb6e72951252409d' into feature/…

8f74cfa

…dudes/table-layout

Revert "Merge commit 'c11ad8ecd2d0d0aba5cd0661eb6e72951252409d' into …

ebb9fc0

…feature/dudes/table-layout" This reverts commit 8f74cfa.

revert last merge and Merge remote-tracking branch 'origin/develop' i…

cb41359

…nto feature/dudes/table-layout

Merge remote-tracking branch 'origin/feature/dudes/table-layout' into…

22659ec

… pvt-logs-cp

begin pvt tables logs - add write in log bool

ff37ac9

duplication log & csv fixed

628f56b

CI correction ( uncrustify, missing doc )

e11a2bf

optionnal added to datatype

a8f9271

update doxygen for ci

9ff17b9

more logs added

d90c8b5

remove static_assert + constness

958d542

Merge remote-tracking branch 'origin/develop' into feature/dudes/tabl…

550ca26

…e-layout

remove unused code

f67e3d9

factorisation log pvt

5b8689a

doxygen added

0fa76e3

remove test parent name with constitutive

bb1ee01

remove unused code + add drawtostring method

aa2f789

uncrustify

8ff95c2

refacto Table first part

957c7d7

refortor still going up (missing doc and some minor refacto)

70d0a01

arng40 and others added 9 commits October 21, 2024 09:41

xsd

b0d731a

Delete src/coreComponents/schema/schema.xsd

f27c101

xsd regenrate

3acc8a1

test ci fix

76a022c

update output domain partition (fix ci)

9c1379f

some renaming

2fafae3

Merge remote-tracking branch 'origin/develop' into feature/dudes/tabl…

cb26312

…e-subs-division

remove unnecessary code

346d8dc

doc

cabdbcb

MelReyCG requested changes Oct 29, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

arng40 added 2 commits October 29, 2024 14:26

missing const

43f9664

renaming & signature & const

2300686

MelReyCG requested changes Oct 29, 2024

View reviewed changes

arng40 added 3 commits October 30, 2024 11:21

improve populate table values

e1af681

renaming, some function refactoring & simplification

b87cee9

cout removed, assert added, doc update

8ea7e56

MelReyCG self-requested a review October 31, 2024 10:26

arng40 added 2 commits October 31, 2024 11:33

move assert

e893ac7

add idx...

e77ea50

MelReyCG requested changes Oct 31, 2024

View reviewed changes

replace string vector on ColumnStructure

ac61e5d

arng40 requested a review from MelReyCG November 5, 2024 13:13

arng40 added 3 commits November 6, 2024 10:04

remove code / doc

d81654c

add assert for transporse

e56d538

Merge remote-tracking branch 'origin/develop' into feature/dudes/tabl…

6332366

…e-subs-division

arng40 requested a review from ryar9534 as a code owner November 8, 2024 10:33

MelReyCG closed this Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Table subs division #3353

feat: Table subs division #3353

arng40 commented Sep 13, 2024 •

edited by MelReyCG

Loading

MelReyCG left a comment

MelReyCG Oct 29, 2024

arng40 Oct 29, 2024

MelReyCG Oct 31, 2024 •

edited

Loading

MelReyCG Oct 29, 2024

arng40 Oct 29, 2024

MelReyCG Oct 31, 2024

MelReyCG Oct 31, 2024 •

edited

Loading

This comment was marked as duplicate.

MelReyCG left a comment

MelReyCG Oct 29, 2024

arng40 Oct 30, 2024

MelReyCG Oct 31, 2024 •

edited

Loading

MelReyCG Nov 12, 2024

MelReyCG Oct 29, 2024

arng40 Oct 30, 2024

MelReyCG Oct 31, 2024

MelReyCG Nov 12, 2024 •

edited

Loading

MelReyCG commented Oct 29, 2024 •

edited

Loading

MelReyCG left a comment

MelReyCG commented Dec 23, 2024

feat: Table subs division #3353

feat: Table subs division #3353

Conversation

arng40 commented Sep 13, 2024 • edited by MelReyCG Loading

MelReyCG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelReyCG Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelReyCG Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

MelReyCG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelReyCG Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelReyCG Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

MelReyCG commented Oct 29, 2024 • edited Loading

MelReyCG left a comment

Choose a reason for hiding this comment

MelReyCG commented Dec 23, 2024

arng40 commented Sep 13, 2024 •

edited by MelReyCG

Loading

MelReyCG Oct 31, 2024 •

edited

Loading

MelReyCG Oct 31, 2024 •

edited

Loading

MelReyCG Oct 31, 2024 •

edited

Loading

MelReyCG Nov 12, 2024 •

edited

Loading

MelReyCG commented Oct 29, 2024 •

edited

Loading