View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0003393OpenFOAMFeaturepublic2019-11-20 13:132019-11-21 13:02
ReporterblttkglAssigned Tohenry 
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformGNU/LinuxOSUbuntuOS Version15.04
Product Versiondev 
Fixed in Versiondev 
Summary0003393: ISAT statistics
DescriptionHey,

Original reporter from https://bugs.openfoam.org/view.php?id=3392

I re-checked the patch provided by Prof. Contino, and apparently there is a bug where he removed the timeIncr variable added to the cpu times, so the library won't compile without undefined variable.

I provide a patch that should work, attached. I unfortunately do not have the dev installed in my workstation, so maybe you could try it.

Pseudocode is:

Initialize time (line 629)
Add the time it takes to search the tree (line 642)
initialize time once more (line 650)
add the time it takes for mechanism reduction (line 658)
add the time for solving chemistry (line 695)
add the time for add (line 722) or grow (line 727)
TagsNo tags attached.

Activities

blttkgl

2019-11-20 13:13

reporter  

TDACChemistryModel.C (23,405 bytes)

henry

2019-11-20 14:27

manager   ~0010928

Thanks for the additional report about the patch, I have reverted it pending a version that compiles and tested in OpenFOAM-dev.

blttkgl

2019-11-21 12:46

reporter   ~0010929

Hey,

I compiled and tested the version I posted earlier on OpenFOAM dev.

Steps to reproduce:

- Run the counterFlowFlame2D_GRI_TDAC tutorial with reduction->off and tEnd=0.25 seconds on 4 processors..
- Use the (quite messy) python script I provide to look at the cpu statistics of the ISAT for old and new way.

You can see in figures (especially bottom middle figure), the cpu_solve is now not included in cpu_add and cpu_grow, which shows more clearly that even utilizing ISAT majority of the time is still spent on solving the ODE, and not binary tree add/grow operation, which is what one might get from the old way.

I also attach a patch that could be applied to the current -dev head.

Best,

Bulut Tekgul

ISAT_statistics_new.png (1,682,601 bytes)
ISAT_statistics_old.png (1,726,831 bytes)
ISAT.patch (2,501 bytes) 
diff --git a/src/thermophysicalModels/chemistryModel/chemistryModel/TDACChemistryModel/TDACChemistryModel.C b/src/thermophysicalModels/chemistryModel/chemistryModel/TDACChemistryModel/TDACChemistryModel.C
index 63accf63c..cb8b4f5d8 100644
--- a/src/thermophysicalModels/chemistryModel/chemistryModel/TDACChemistryModel/TDACChemistryModel.C
+++ b/src/thermophysicalModels/chemistryModel/chemistryModel/TDACChemistryModel/TDACChemistryModel.C
@@ -647,8 +647,8 @@ Foam::scalar Foam::TDACChemistryModel<ReactionThermo, ThermoType>::solve
         // (it will either expand the current data or add a new stored point).
         else
         {
-            // Store total time waiting to attribute to add or grow
-            scalar timeTmp = clockTime_.timeIncrement();
+            // Reset the time
+            clockTime_.timeIncrement();
 
             if (reduced)
             {
@@ -656,9 +656,7 @@ Foam::scalar Foam::TDACChemistryModel<ReactionThermo, ThermoType>::solve
                 mechRed_->reduceMechanism(pi, Ti, c, celli);
                 nActiveSpecies += mechRed_->NsSimp();
                 nAvg++;
-                scalar timeIncr = clockTime_.timeIncrement();
-                reduceMechCpuTime_ += timeIncr;
-                timeTmp += timeIncr;
+                reduceMechCpuTime_ += clockTime_.timeIncrement();
             }
 
             // Calculate the chemical source terms
@@ -695,9 +693,7 @@ Foam::scalar Foam::TDACChemistryModel<ReactionThermo, ThermoType>::solve
             }
 
             {
-                scalar timeIncr = clockTime_.timeIncrement();
-                solveChemistryCpuTime_ += timeIncr;
-                timeTmp += timeIncr;
+                solveChemistryCpuTime_ += clockTime_.timeIncrement();
             }
 
             // If tabulation is used, we add the information computed here to
@@ -724,12 +720,12 @@ Foam::scalar Foam::TDACChemistryModel<ReactionThermo, ThermoType>::solve
                 if (growOrAdd)
                 {
                     this->setTabulationResultsAdd(celli);
-                    addNewLeafCpuTime_ += clockTime_.timeIncrement() + timeTmp;
+                    addNewLeafCpuTime_ += clockTime_.timeIncrement();
                 }
                 else
                 {
                     this->setTabulationResultsGrow(celli);
-                    growCpuTime_ += clockTime_.timeIncrement() + timeTmp;
+                    growCpuTime_ += clockTime_.timeIncrement();
                 }
             }
ISAT.patch (2,501 bytes)

blttkgl

2019-11-21 12:46

reporter   ~0010930

python code

printCPUT.py (10,721 bytes)

henry

2019-11-21 13:02

manager   ~0010931

Thanks for the correction

Resolved by commit e18c81e84ff3cd5f293001585489d966d7189dd0

Issue History

Date Modified Username Field Change
2019-11-20 13:13 blttkgl New Issue
2019-11-20 13:13 blttkgl File Added: TDACChemistryModel.C
2019-11-20 14:27 henry Note Added: 0010928
2019-11-21 12:46 blttkgl File Added: ISAT_statistics_new.png
2019-11-21 12:46 blttkgl File Added: ISAT_statistics_old.png
2019-11-21 12:46 blttkgl File Added: ISAT.patch
2019-11-21 12:46 blttkgl Note Added: 0010929
2019-11-21 12:46 blttkgl File Added: printCPUT.py
2019-11-21 12:46 blttkgl Note Added: 0010930
2019-11-21 13:01 henry Category Bug => Feature
2019-11-21 13:02 henry Assigned To => henry
2019-11-21 13:02 henry Status new => resolved
2019-11-21 13:02 henry Resolution open => fixed
2019-11-21 13:02 henry Fixed in Version => dev
2019-11-21 13:02 henry Note Added: 0010931