Adding SourceFluxStatistics #2954

MelReyCG · 2024-01-25T16:37:56Z

The goal of this PR is to add a SourceFluxStatistics component which aggregate the producted mass & rate on a SourceFlux set.

Log Level 1 outputs the sum of all SourceFluxStatistics produced rate & mass,
Log Level 2 details values for each SourceFluxStatistics,
Log Level 3 details values for each region.

Here is the typical log output for a multi-phase simulation, with a log level of 2 :

Rank 0: SourceFluxStatistics timeStepFluxStats (of sourceFlux, in all_regions): Producing on 1 elements
Rank 0: SourceFluxStatistics timeStepFluxStats (of sourceFlux, in all_regions): Produced mass = [-280.5, 0] kg
Rank 0: SourceFluxStatistics timeStepFluxStats (of sourceFlux, in all_regions): Production rate = [-0.561, 0] kg/s
Rank 0: SourceFluxStatistics timeStepFluxStats (of sinkFlux, in all_regions): Producing on 1 elements
Rank 0: SourceFluxStatistics timeStepFluxStats (of sinkFlux, in all_regions): Produced mass = [0, 31] kg
Rank 0: SourceFluxStatistics timeStepFluxStats (of sinkFlux, in all_regions): Production rate = [0, 0.062] kg/s
Rank 0: SourceFluxStatistics timeStepFluxStats (of flux_set, in all_regions): Producing on 2 elements
Rank 0: SourceFluxStatistics timeStepFluxStats (of flux_set, in all_regions): Produced mass = [-280.5, 31] kg
Rank 0: SourceFluxStatistics timeStepFluxStats (of flux_set, in all_regions): Production rate = [-0.561, 0.062] kg/s

Note: If the PeriodicEvent that execute the SourceFluxStatistics has a frequency such as multiple time-step are encountered, the statistics are aggregated (masses -> sum, rates -> mean).

I've also added a test which checks if the statistics output of the SourceFluxStatistics and SinglePhaseStatistics::RegionStatistics::totalMass are consistent with the provided production rates table.
The test checks if the statistics are right over the whole simulation, but also for each timesteps. It will crash if a timestep does not run. Thus, it tests if the PeriodicEvent functions as expected (is it the only test to do so ?).

…atistics of SourceFluxBoundaryCondition (WIP)

… in a more specific PR later)

…flux-stats

- finalize architecture - add a way to accumulate statistics over multiple timesteps - test case runs correctly - added a way to know the region of a subregion - cleanup - completed some wrappers descriptions

…S-DEV/GEOS into feature/rey/sourceflux-stats

src/coreComponents/unitTests/fluidFlowTests/testFluidStatistics.cpp

dkachuma · 2024-03-12T16:06:37Z

src/coreComponents/fieldSpecification/SourceFluxStatistics.hpp

This looks like a task that should belong to physicsSolvers and not quite a fieldSpecification.

So the solvers would expose the producted rates & masses for each SourceFlux, and then the SourceFluxStatistics would access to it?

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

dkachuma · 2024-03-12T16:46:27Z

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

+    {
+      registerWrappedStats( mesh, fluxName, viewKeyStruct::allRegionWrapperString() );
+
+      mesh.getElemManager().forElementRegions( [&]( ElementRegionBase & region )


If the logLevel is low enough, then we don't need to create these wrappers.

I use them to collect stats from the SinglePhaseBase::applySourceFluxBC() methods that have no knowledge of the regions. Can I keep them to collect the data or should I proceed differently?

TotoGaz

That looks like a nice feature.
A question about the display though.

TotoGaz · 2024-03-20T17:29:07Z

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

+  }
+}
+
+bool SourceFluxStatsAggregator::execute( real64 const GEOS_UNUSED_PARAM( time_n ),


Hi, I'd be interested in decoupling the gathering/aggregation of the data from its output.
Here we impose log and csv, for example. There's no reason not to use the CSV for other types of data, and to impose CSV for the source flux data.

@MelReyCG , I had a quick chat with @arng40 about that.

Just to notify everyone about the conclusion of our chat on this subject:

There are currently a lot of these log / CSV output in the statistics classes,

Statistics output must be unified, and not strictly bound to CSV,

This output refactoring is not the subject of this PR, but is a task that I have got in my roadmap.

An Outputs new node could be created to output those stats, maybe something like:

<Outputs> <StatsOutput name="mySourceFluxesOutput" source="mySourceFluxesStats" outputFile="sourceFluxStats.csv" />  </Outputs>

…ies folder)

…flux-stats

CusiniM · 2024-04-05T00:29:49Z

src/coreComponents/common/Units.hpp

+    case Viscosity:       return "Pa*s";
+    case Enthalpy:        return "J/kg";
+    case Density:         return "kg/m3";
+    case Solubility:      return "g/L";
+    case Mass:            return "kg";
+    case Mole:            return "mol";
+    case MassRate:        return "kg/s";
+    case MoleRate:        return "mol/s";


Units are not really strongly enforced in GEOS. One could use any set of units as long as they are consistent. For example a user can decide to specify pressures in MPa an adjust all other properties. I would say that we either properly introduce a unit parser or these units can easily be false.

About this header, it is in S.I. by default (following the GEOS convention). Some units can be in other systems as you can see for the temperatures (°C rather than °K), and if someone want to add more, I think it should come with conversion functions.
I strongly advise to add comments on all non-S.I. variables / method, it would be less error-prone and would help when implementing a parsing unit system or when adding new outputs.

About adding unit parsing system, it is on my roadmap. Feel free to add suggestions on my EPIC.

TotoGaz · 2024-05-04T01:26:54Z

src/coreComponents/physicsSolvers/fluidFlow/CompositionalMultiphaseStatistics.cpp

@@ -305,7 +304,7 @@ void CompositionalMultiphaseStatistics::computeRegionStatistics( real64 const ti
                                        subRegionImmobilePhaseMass.toView(),
                                        subRegionComponentMass.toView() );

-    ElementRegionBase & region = elemManager.getRegion( subRegion.getParent().getParent().getName() );
+    ElementRegionBase & region = elemManager.getRegion( ElementRegionBase::getParentRegion( subRegion ).getName() );


That's better!

TotoGaz · 2024-05-04T01:34:24Z

src/coreComponents/unitTests/testingUtilities/TestingTasks.hpp

+ * As this Group is designed for developpers and not user oriented, it should not appear in the documentation nor in the xsd.
+ * Question: could this task be used in the integratedTests ?
+ */
+class TimeStepChecker : public TaskBase


@CusiniM What do you think of this. I believe that's interesting.

TotoGaz · 2024-05-04T01:35:37Z

src/coreComponents/physicsSolvers/SolverBase.hpp

@@ -734,6 +734,7 @@ class SolverBase : public ExecutableGroup
  virtual bool registerCallback( void * func, const std::type_info & funcType ) final override;

  SolverStatistics & getSolverStatistics() { return m_solverStatistics; }
+  SolverStatistics const & getSolverStatistics() const { return m_solverStatistics; }


Who's using the non-const version?
If you can, can you remove it or use a setter instead?

It is used at src/coreComponents/physicsSolvers/multiphysics/CoupledSolver.hpp:431, 491, 506, I think the non-const version is useful, should I remove the const one?

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

TotoGaz · 2024-05-04T01:41:08Z

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

+        regionStats.stats() = StatData();
+
+        forAllSubRegionStatsWrappers( region, regionStats.getFluxName(),
+                                      [&] ( ElementSubRegionBase &, WrappedStats & subRegionStats )


4 nested lambda functions, I think that you tie the record 🤣 😉

TotoGaz · 2024-05-04T01:42:40Z

src/coreComponents/fieldSpecification/SourceFluxStatistics.cpp

+  }
+
+  // combine period results from all MPI ranks
+  m_stats.mpiReduce();


You need to be sure that this is called by all the ranks. Is it the case?

Indeed, I tested that all ranks send & combine how much the SourceFlux produced / injected (possibly 0).

src/coreComponents/fieldSpecification/SourceFluxStatistics.hpp

…rey/sourceflux-stats

MelReyCG added 20 commits December 21, 2023 15:43

added region mass stats

d73ac6d

added region temperature stats

909a1c5

Logging debug info - to be removed laster

8bee3cf

better debug output

256caeb

annotations

0ec69b5

cleaning

67d8a7e

Output flux stats in log

27bec78

completed previous commit

f89e2d8

Add the logLevel viewkey & description

e0592ed

Add a way to expend the description of a wrapper

078e6ed

Intermediate progress to properly store the SourceFlux stats

5b8f213

adding the SourceFluxStatistics Task which will allow to aggregate st…

953ec3a

…atistics of SourceFluxBoundaryCondition (WIP)

simplify description to avoid regenerating a lot of .rst (to be added…

1a9fe0c

… in a more specific PR later)

Merge remote-tracking branch 'origin/develop' into feature/rey/source…

867a20c

…flux-stats

missed a few details

814fd34

forgotten generated files...

818fcc2

SourceFlluxStatistics now compiles

a5d762b

generated docs

28a7730

SourceFluxStatistics now runs:

4e71130

- finalize architecture - add a way to accumulate statistics over multiple timesteps - test case runs correctly - added a way to know the region of a subregion - cleanup - completed some wrappers descriptions

few edits

cc55358

MelReyCG self-assigned this Jan 25, 2024

MelReyCG added the type: feature New feature or request label Jan 25, 2024

MelReyCG added 8 commits January 26, 2024 15:26

uncrustify

69c6918

architecture adaptations to expose values to unit tests

a164ece

added singlephase test

badfc4e

removed debug infos

23e41fc

unit test: code layout, removed debug info

d59318a

unit test: code layout, removed debug info, xmlInput improvements

db72798

Merge branch 'feature/rey/sourceflux-stats' of https://github.com/GEO…

232106b

…S-DEV/GEOS into feature/rey/sourceflux-stats

Added multiphase xml input

8477262

MelReyCG added 2 commits March 5, 2024 17:26

solver precision

dc5b827

avoiding exact 0 / 1 values in test

7f9097c

MelReyCG requested review from dkachuma and rrsettgast March 8, 2024 14:13

MelReyCG mentioned this pull request Mar 8, 2024

CompositionalMultiphaseFVM crash in Debug build #3031

Closed

paveltomin reviewed Mar 8, 2024

View reviewed changes

src/coreComponents/unitTests/fluidFlowTests/testFluidStatistics.cpp Outdated Show resolved Hide resolved

paveltomin reviewed Mar 8, 2024

View reviewed changes

src/coreComponents/unitTests/fluidFlowTests/testFluidStatistics.cpp Outdated Show resolved Hide resolved

MelReyCG added 2 commits March 8, 2024 16:24

removed parameter as they are to defualt values

de20aba

renaming testFluidStatistics to testFlowStatistics

9f78b8d

paveltomin approved these changes Mar 9, 2024

View reviewed changes

dkachuma approved these changes Mar 12, 2024

View reviewed changes

TotoGaz reviewed Mar 20, 2024

View reviewed changes

MelReyCG added 7 commits April 3, 2024 15:12

removing the 4th log level

25bc30f

missing inlines

f56b2f6

xsd / rst changes

37b3c97

moving the TimeStepChecker in its own file (& created a testingUtilit…

3165aef

…ies folder)

Merge remote-tracking branch 'origin/develop' into feature/rey/source…

9905ae1

…flux-stats

moving some definition to cpp

3883d39

uncrustify :s

8f72e9f

MelReyCG requested a review from TotoGaz April 4, 2024 07:28

CusiniM reviewed Apr 5, 2024

View reviewed changes

TotoGaz approved these changes May 4, 2024

View reviewed changes

MelReyCG added 6 commits June 4, 2024 16:15

few adjustments

43fe9f4

Merge commit '79856a0e653a508b4fe4dcd8c582fcdc2838b756' into feature/…

34fc78d

…rey/sourceflux-stats

added an header due to incomplete DomainPartition type

cf15bb3

by default, SourceFluxStat targets al SourceFlux + adapted test

b051ed1

generated files

cf0271a

Added star character * in group name reference regex.

b0c7fe6

MelReyCG added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Jun 5, 2024

paveltomin added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding SourceFluxStatistics #2954

Adding SourceFluxStatistics #2954

MelReyCG commented Jan 25, 2024 •

edited

dkachuma Mar 12, 2024

MelReyCG Mar 28, 2024

dkachuma Mar 12, 2024

MelReyCG Mar 28, 2024

TotoGaz left a comment

TotoGaz Mar 20, 2024

MelReyCG Mar 28, 2024 •

edited

CusiniM Apr 5, 2024

MelReyCG Apr 8, 2024

TotoGaz May 4, 2024

TotoGaz May 4, 2024

TotoGaz May 4, 2024

MelReyCG Jun 4, 2024 •

edited

TotoGaz May 4, 2024

TotoGaz May 4, 2024

MelReyCG Jun 4, 2024

Adding SourceFluxStatistics #2954

Are you sure you want to change the base?

Adding SourceFluxStatistics #2954

Conversation

MelReyCG commented Jan 25, 2024 • edited

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

TotoGaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MelReyCG Mar 28, 2024 • edited

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

Choose a reason for hiding this comment

MelReyCG Jun 4, 2024 • edited

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 commented Jan 25, 2024 •

edited

MelReyCG Mar 28, 2024 •

edited

MelReyCG Jun 4, 2024 •

edited