GEOS icon indicating copy to clipboard operation
GEOS copied to clipboard

Adding SourceFluxStatistics

Open MelReyCG opened this issue 1 year ago • 13 comments

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 ?).

MelReyCG avatar Jan 25 '24 16:01 MelReyCG

please double check that logic is consistent with useMass flag (default is 0)

paveltomin avatar Feb 14 '24 16:02 paveltomin

please double check that logic is consistent with useMass flag (default is 0)

and time step chop

paveltomin avatar Feb 14 '24 22:02 paveltomin

@paveltomin

Timestep cuts were tested by the checkMultiPhaseFluxStatistics test (this simulation has 3 sub-timesteps). I added a guard (testInputs.requireSubTimeStep) so it will keep being tested with timesteps.

About useMass, the checkMultiPhaseFluxStatistics simulation does not converge anymore when I set it to 1. I am probably missing how to manage this component. Are the SourceFlux units in mole by default ? I'll investigate tomorrow.

Side question : do you think I should add CSV export for this component ?

MelReyCG avatar Feb 15 '24 16:02 MelReyCG

About useMass, the checkMultiPhaseFluxStatistics simulation does not converge anymore when I set it to 1. I am probably missing how to manage this component. Are the SourceFlux units in mole by default ? I'll investigate tomorrow.

That's the tricky part, up to my understanding SourceFlux units depend on useMass flag. If useMass=0 then units are moles, if useMass=1 then units are mass.

Side question : do you think I should add CSV export for this component ?

I found CSV output for flow and mechanics very convenient and use it all the time, also I think @jhuang2601 started using it. It would be nice to have it for SourceFlux too yes, but it can be done in a separate PR.

paveltomin avatar Feb 15 '24 16:02 paveltomin

Timestep cuts were tested by the checkMultiPhaseFluxStatistics test (this simulation has 3 sub-timesteps). I added a guard (testInputs.requireSubTimeStep) so it will keep being tested with timesteps.

You test one new code with another new code :)

Your current implementation is tricky and only valid for constant flux value because if I understood correctly the statistics is being accumulated over the newton iterations while the real value that matters is the final one, when the iterations converged. I don't see any zeroing-out of accumulated quantities when time step cut happens but I am probably blind. My suspicion for time step cut is similar - only works because the flux value is constant, but fundamental logic behind is slippery.

paveltomin avatar Feb 15 '24 16:02 paveltomin

Codecov Report

Attention: Patch coverage is 88.23529% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 56.17%. Comparing base (82879a1) to head (8f72e9f).

Files Patch % Lines
src/coreComponents/common/Units.hpp 10.00% 18 Missing :warning:
...onents/fieldSpecification/SourceFluxStatistics.cpp 88.23% 18 Missing :warning:
...s/fluidFlow/ReactiveCompositionalMultiphaseOBL.cpp 0.00% 11 Missing :warning:
...ts/unitTests/fluidFlowTests/testFlowStatistics.cpp 97.48% 6 Missing :warning:
...onents/fieldSpecification/SourceFluxStatistics.hpp 91.30% 4 Missing :warning:
...nents/physicsSolvers/fluidFlow/SinglePhaseBase.cpp 82.35% 3 Missing :warning:
...onents/physicsSolvers/fluidFlow/FlowSolverBase.hpp 0.00% 2 Missing :warning:
...sSolvers/fluidFlow/CompositionalMultiphaseBase.cpp 93.33% 1 Missing :warning:
...rs/fluidFlow/CompositionalMultiphaseStatistics.cpp 66.66% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2954      +/-   ##
===========================================
+ Coverage    53.22%   56.17%   +2.94%     
===========================================
  Files          988      993       +5     
  Lines        83496    84013     +517     
===========================================
+ Hits         44443    47193    +2750     
+ Misses       39053    36820    -2233     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 15 '24 17:02 codecov[bot]

@paveltomin

I've added the CSV export and try to make the code of SourceFluxStatsAggregator::WrappedStats::gatherTimeStepStats() clearer. Do you think this method is missing something about timestep cuts? Or do you think I should go for another way to keep track of data accumulation.

You test one new code with another new code :)

I did not understand if you are thinking that is a good thing. I made a typo it the comment you quoted -> What I did was to add a check in my unit test to ensure that sub-timesteps are being tested.

MelReyCG avatar Feb 16 '24 16:02 MelReyCG

I've added the CSV export and try to make the code of SourceFluxStatsAggregator::WrappedStats::gatherTimeStepStats() clearer. Do you think this method is missing something about timestep cuts? Or do you think I should go for another way to keep track of data accumulation.

Thanks for CSV and if you say timestep cut is ok, I trust that, thanks for checking and refining the code

You test one new code with another new code :)

I did not understand if you are thinking that is a good thing. I made a typo it the comment you quoted -> What I did was to add a check in my unit test to ensure that sub-timesteps are being tested.

I just mean that one could potentially do the same mistake in the main code and in the test code, so unit test may not help catching it

Would you mind adding that output into one of the xmls?

paveltomin avatar Feb 16 '24 17:02 paveltomin

@paveltomin

I just mean that one could potentially do the same mistake in the main code and in the test code, so unit test may not help catching it

Oh I see! To explain my test, I prepared some expected data and a xml input and check that the simulation outputs statistics that are consistent with the expected ones, so the test is not dependent of the internal code. I also chek if the count of timestep is as expected. And now I check if at least one timestep cut occurs, in order to see if everything goes right in that case too.

Would you mind adding that output into one of the xmls?

Which XMLs output are you talking about ?

MelReyCG avatar Feb 19 '24 09:02 MelReyCG

Would you mind adding that output into one of the xmls?

Which XMLs output are you talking about ?

I mean add this new SourceFluxStatistics output to one of the existing XMLs

paveltomin avatar Feb 19 '24 15:02 paveltomin

Ok I will add one or more SourceFluxStatistics in the exemples.

useMass has indeed an influence on the SourceFlux units 👍 :

  • I fixed the SourceFluxStatistics and added a (temporary) test to compare with mass and mole,
  • I added descriptions of the wrapper about the fluxes units.
  • I added a way of getting the amount of fluid unit on FlowSolverBase, and uniformized a bit with the other statistics classes.
  • I verified the SinglePhaseStatistics units.

I don't know why, but the same test simulation in mass and mole are behaving differently (MultiPhaseFluxStatisticsTestMol has a timestep cut whereas MultiPhaseFluxStatisticsTestMass haven't any), here is the log : test2.txt

MelReyCG avatar Feb 21 '24 15:02 MelReyCG

I don't know why, but the same test simulation in mass and mole are behaving differently (MultiPhaseFluxStatisticsTestMol has a timestep cut whereas MultiPhaseFluxStatisticsTestMass haven't any), here is the log : test2.txt

this is normal, since the amount injected can be very different, solver can be more stressed

paveltomin avatar Feb 21 '24 22:02 paveltomin

Ok I see. Thanks a lot for your inputs and review !

MelReyCG avatar Feb 22 '24 08:02 MelReyCG