GEOS
GEOS copied to clipboard
Adding SourceFluxStatistics
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 ?).
please double check that logic is consistent with useMass flag (default is 0)
please double check that logic is consistent with useMass flag (default is 0)
and time step chop
@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 ?
About
useMass
, thecheckMultiPhaseFluxStatistics
simulation does not converge anymore when I set it to 1. I am probably missing how to manage this component. Are theSourceFlux
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.
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.
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
).
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.
@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.
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
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 ?
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
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
I don't know why, but the same test simulation in mass and mole are behaving differently (
MultiPhaseFluxStatisticsTestMol
has a timestep cut whereasMultiPhaseFluxStatisticsTestMass
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
Ok I see. Thanks a lot for your inputs and review !