View Issue Details

IDProjectCategoryView StatusLast Update
0000815OpenFOAMBugpublic2015-03-22 09:53
Reporterprojectionist Assigned Tohenry  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformUbuntuOSLinuxOS Version12.04
Summary0000815: Time step directory naming error
DescriptionIf I continue a simulation, e.g. it first it ran to 20s then I continue to 60s, the time step directory names get a bit weird.

OpenFOAM issues some warning messages about increasing the timePrecision.

You find my controlDict attached. I also included some lines of the solver output.

The time step directory names look like this:

118.5000000003
119.0000000003
119.5000000003
120.0000000003
60
60.5
61
61.5

In this case the simulation was run till t=60s and then continued to t=120s.
Steps To Reproducerestart a simulation.

Build : 2.1.x-82fc6e06f8b3
Build : 2.1.x-0038d7805653

In both cases I made simulation with fixed time step.
Additional InformationCourant Number mean: 0.000730585 max: 0.00230961
Max Ur Courant Number = 0.00375471
Time = 20.0001

--> FOAM Warning :
    From function Time::operator++()
    in file db/Time/Time.C at line 1024
    Increased the timePrecision from 6 to 7 to distinguish between timeNames at time 20
MULES: Solving for alpha1
/* solver output */
ExecutionTime = 3.23 s ClockTime = 4 s

Courant Number mean: 0.000730578 max: 0.00230962
Max Ur Courant Number = 0.00375456
--> FOAM Warning :
    From function Time::operator++()
    in file db/Time/Time.C at line 1024
    Increased the timePrecision from 7 to 17 to distinguish between timeNames at time 20.0001
Time = 20.000100000000003

MULES: Solving for alpha1
/* solver output */
ExecutionTime = 5 s ClockTime = 6 s

Courant Number mean: 0.000730579 max: 0.00230963
Max Ur Courant Number = 0.00375451
Time = 20.000150000000005

MULES: Solving for alpha1
TagsNo tags attached.

Activities

projectionist

2013-04-15 08:32

reporter  

controlDict (1,478 bytes)   
/*--------------------------------*- C++ -*----------------------------------*\
| =========                 |                                                 |
| \\      /  F ield         | OpenFOAM: The Open Source CFD Toolbox           |
|  \\    /   O peration     | Version:  2.1.x                                 |
|   \\  /    A nd           | Web:      www.OpenFOAM.org                      |
|    \\/     M anipulation  |                                                 |
\*---------------------------------------------------------------------------*/
FoamFile
{
    version     2.0;
    format      ascii;
    class       dictionary;
    location    "system";
    object      controlDict;
}
// * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * //

application     twoPhaseEulerFoam;

//startFrom       startTime;
startFrom       latestTime

startTime       0;

stopAt          endTime;

endTime         20;

deltaT          0.00005;

writeControl    runTime;
//writeControl    adjustableRunTime;

writeInterval   0.5;

purgeWrite      0;

writeFormat     ascii;

writePrecision  6;

writeCompression uncompressed;

timeFormat      general;

timePrecision   6;

runTimeModifiable off;

adjustTimeStep  no;

maxCo           0.5;

maxDeltaT       1;

functions
{
    probes1
	{
		type probes;

		functionObjectLibs ("libsampling.so");

		dictionary probesDict;
	}
}


// ************************************************************************* //
controlDict (1,478 bytes)   

user258

2013-07-07 19:05

  ~0002310

See also: http://openfoam.org/mantisbt/view.php?id=514

user4

2013-07-08 10:04

  ~0002311

You start off from 20 with a deltaT of 0.00005 so you'll need at least 2 (leading digits) + 5 (trailing digits) time precision.

I do not understand the second, 7 to 17 digits precision extension - do you have a 22x testcase?

wyldckat

2015-03-06 14:22

updater   ~0003992

This issue seems related to this one: http://www.openfoam.org/mantisbt/view.php?id=1557

wyldckat

2015-03-07 19:58

updater  

runTimeSteps.tar.gz (4,009 bytes)

wyldckat

2015-03-07 20:12

updater   ~0004003

OK, step 1: reproduce problem - complete!

Attached is the file "runTimeSteps.tar.gz", which has packaged the test utility "runTimeSteps" (ready for "applications/test") which compiles to the binary named "Test-runTimeSteps". Do feel free to add it to the test folder, since this is extremely basic stuff (and I snipped most of it anyway from "simpleFoam" and "pimpleFoam")!

How to use it, after unpacking:

  wmake
  cd pitzDailyDummy
  blockMesh

Then:
  Test-runTimeSteps -simple > log
  Test-runTimeSteps -simple > log2

Or:
  Test-runTimeSteps -pimple > log
  Test-runTimeSteps -pimple > log2

The reason for the two types was for the simple reasoning of "what if something is different between simple and pimple". This way it's ready for adding in a few more tests, if need be (e.g. deltaT affected by "CourantNo").


The reason why we should run twice is because of these settings:
  startFrom latestTime;
  startTime 0;
  stopAt endTime;
  endTime 100;
  deltaT 0.00005;
  writeControl runTime;
  writeInterval 0.54325;

The first run will end at "99.958" and everything looks fine. The second run will reproduce the reported problem:

    Create mesh for time = 99.958


    SIMPLE: no convergence criteria found. Calculations will run for 100 steps.


    Starting time loop

    Time = 99.9581

    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1055
        Increased the timePrecision from 6 to 17 to distinguish between timeNames at time 99.9581
    Time = 99.958100000000002

    Time = 99.958150000000003


Notice anything else strange? "99.95805" is missing in between load and 1st step! Which probably explains why the iterator went "uh oh, gotta fix this" - but did it way too late and messed it up ;)

---------------
Next step: figuring out the fix for this.

wyldckat

2015-03-07 21:35

updater  

Time.C.v1 (30,624 bytes)

wyldckat

2015-03-07 21:35

updater  

issue815.patch.v1 (1,812 bytes)

wyldckat

2015-03-07 21:47

updater   ~0004004

Steps 2 and 3: Diagnosis and first possible fix - completed!
Relative to commit ade709a58 from 2.3.x.


Diagnosis: Comparing the old name with the new name, for the time snapshots, results in not taking into account the limitations of the number rounding up/down algorithm. In the example from the previous comment, "99.95805" is rounded up to "99.9581", which results in colliding with the actual "99.9581" in the next iteration.


Solution: Compare the actual values, namely the currently defined value - reinterpreted from string - versus the previous value and compare the difference to the desired "deltaT".


First possible fix: Attached are two files:

 - "Time.C.v1" - the complete modified file "src/OpenFOAM/db/Time/Time.C", since patches aren't always easy to use.

 - "issue815.patch.v1" - the patch file that should result in the file "Time.C.v1", for easier on-the-fly inspection.

This fix doesn't seem to the best possible solution, but right now I can't deduce a better one. In addition, this code needs aesthetic revising, since I wasn't certain how to follow the guidelines in the situations of long "if" and "while" statements.


If it's possible that we could access directly to the rounded up/down value at the time of formatting to "word", it would save up a few cpu cycles...

henry

2015-03-07 22:02

manager   ~0004006

This is close to the way I have been thinking about this problem, the only difference is that I think we need a better way of defining what an appropriate smallest time-step for the case might be (sqrt(SMALL) in your code if I understood it correctly). While it might be OK to define a default I think it sholud be possible to over-ride it and this also would allow for the case to terminate if the time-step is reduced below this limit.

wyldckat

2015-03-07 22:20

updater   ~0004007

OK, I'll leave that detail to you.
I'm finishing up a second additional patch, which relates mainly to issue #1557 and a little bit to this one as well.

wyldckat

2015-03-07 22:43

updater   ~0004008

Well... looks like the 2nd patch I tried to created that is meant for #1557 cannot work properly without adding an additional argument to the "Time" constructors for checking if the folder exists. It's provided as attachment "issue815.patch.v2".

Perhaps it makes more sense to add a new method to "Time" and then add this check to "createTime.H" that is used by most solvers?

wyldckat

2015-03-07 22:43

updater  

issue815.patch.v2 (696 bytes)

henry

2015-03-07 23:53

manager   ~0004010

I don't see the need to change the code for #1557, if the time directory is not present the fields cannot be read from it and the code stops. Perhaps the message could be improved a bit as in your change to Time but I don't think Time should attempt the check as the directory may not be needed.

henry

2015-03-08 10:50

manager   ~0004011

Building on your idea of checking of the existence of the time directory why don't we instead attempt to work out what the precision needs to be set to find it:

    // Check if time directory exists
    // If not increase time precision to see if it is formatted differently.
    if (!exists(timePath(), false))
    {
        int oldPrecision = precision_;
        bool found = false;
        for (precision_=oldPrecision; precision_<20; precision_++)
        {
            // Update the time formatting
            setTime(startTime_, 0);

            // Check the existence of the time directory with the new format
            found = exists(timePath(), false);

            if (found) break;
        }

        if (found)
        {
            WarningIn("Time::setControls()")
                << "Increasing the timePrecision from " << oldPrecision
                << " to " << precision_
                << " to support the formatting of the current time directory "
                << timeName() << nl << endl;
        }
        else
        {
            // Could not find time directory so assume it is not present
            precision_ = oldPrecision;

            // Revert the time formatting
            setTime(startTime_, 0);
        }
    }

Realistically what should the maximum precision be for this? I think that 100 use elsewhere is unnecessarily large. Probably 10 to cover most applications but should be be say 20 as in the above?

wyldckat

2015-03-08 11:42

updater   ~0004012

I believe that maximum precision should be tied to the machine epsilon for the current scalar precision: http://en.wikipedia.org/wiki/Machine_epsilon

 - SP: minimum required: 7. But perhaps 9-10?

 - DP: minimum required: 16. Again, perhaps 19-20?

A bit of extra precision would only be useful for corner cases like, e.g. for SP: 1.23456789999


I forgot to mention yesterday that the main problem with the 2nd patch is that several warnings are issued in cases where the folders are still being constructed, e.g. when using decomposePar. Which is why I suggested that the check should perhaps be done separately, namely only when we really need to folder to already exist.


OK, now I see it. Yes, your code should work well and it will only warn us if the precision needed to be upped.

henry

2015-03-08 11:51

manager   ~0004013

I am working on your first patch at the moment. Having run your test case and put in some diagnostics I see exactly what is going on and how your patch resolves it -- thanks for effort. I hope to put something together which is a bit more elegant and obvious but it is not so easy converting word representations back to scalars as you already found.

Your suggestion on SP vs DP makes sense, basically (3 -log10(SMALL)) or similar

wyldckat

2015-03-08 11:52

updater   ~0004014

OK, found a corner case: The precision has to be searched for in the inverse direction, because of situations like these:

 - 99.958
 - 99.95800001

It will stop searching when it finds the first folder.

henry

2015-03-08 11:57

manager   ~0004015

Good point, I will finish sorting out a static member specifying the maximum precision and then reverse the loop.

wyldckat

2015-03-08 12:04

updater   ~0004016

Tested it just now... doesn't work as intended :(
Inverse order leads to too much precision, which can lead to unwanted inaccuracies...

    Not found: "/home/ofuser/OpenFOAM/OpenFOAM-2.3.x/applications/test/runTimeSteps/pitzDailyDummy/1e+02"
    Patch checked: "/home/ofuser/OpenFOAM/OpenFOAM-2.3.x/applications/test/runTimeSteps/pitzDailyDummy/99.958", 1
    Patch checked: "/home/ofuser/OpenFOAM/OpenFOAM-2.3.x/applications/test/runTimeSteps/pitzDailyDummy/99.9579999999999984", 0
    --> FOAM Warning :
        From function Time::setControls()
        in file db/Time/Time.C at line 225
        Increasing the timePrecision from 2 to 18 to support the formatting of the current time directory 99.9579999999999984

    Create mesh for time = 99.9579999999999984

Unless it searches for all possible precisions...

henry

2015-03-08 12:07

manager   ~0004017

OK, I will take it out.

wyldckat

2015-03-08 12:11

updater   ~0004018

Tested just now... here's what seemed to worked well:

    // Check if time directory exists
    // If not increase time precision to see if it is formatted differently.
    if (!exists(timePath(), false))
    {
        int oldPrecision = precision_;
        bool found = false;
        int precisionNeeded = -1;

        for (
             precision_ = 3-log10(SMALL);
             precision_ > oldPrecision;
             precision_--
            )
        {
            // Update the time formatting
            setTime(startTime_, 0);

            // Check the existence of the time directory with the new format
            found = exists(timePath(), false);

            if (found)
            {
              precisionNeeded = precision_;
            }
        }
        
        if (precisionNeeded>0)
        {
            precision_ = precisionNeeded;

            WarningIn("Time::setControls()")
                << "Increasing the timePrecision from " << oldPrecision
                << " to " << precision_
                << " to support the formatting of the current time directory "
                << timeName() << nl << endl;
        }
        else
        {
            // Could not find time directory so assume it is not present
            precision_ = oldPrecision;

            // Revert the time formatting
            setTime(startTime_, 0);
        }
    }
    

Searching in reverse has the advantage of calling "3-log10(SMALL)" only once.

henry

2015-03-08 12:14

manager   ~0004019

I have just created a maxPrecision_ static member so evaluation is not an issue.
Feel free to search in either direction.

wyldckat

2015-03-08 12:21

updater   ~0004020

Last edited: 2015-03-08 12:22

Searching in direct order based in the code from my previous comment:

             precision_ = oldPrecision+1;
             precision_ <= 3-log10(SMALL);
             precision_++

fails to properly identify the minimal precision needed. Looks like reverse order is better for a simpler and more efficient code? Otherwise more "if"s have to be used, as far as I can figure out.

henry

2015-03-08 12:24

manager   ~0004021

Yes, I think searching in reverse-order is more logical. I am building the changes with maxPrecision_ at the moment; takes a while....

wyldckat

2015-03-08 12:27

updater   ~0004022

OK, with any luck this is the final code that works as intended... it was missing a setTime in the last if block:


    // Check if time directory exists
    // If not increase time precision to see if it is formatted differently.
    if (!exists(timePath(), false))
    {
        int oldPrecision = precision_;
        bool found = false;
        int precisionNeeded = -1;

        for (
             precision_ = 3-log10(SMALL);
             precision_ > oldPrecision;
             precision_--
            )
        {
            // Update the time formatting
            setTime(startTime_, 0);

            // Check the existence of the time directory with the new format
            found = exists(timePath(), false);

            if (found)
            {
               precisionNeeded = precision_;
            }
        }
        
        if (precisionNeeded>0)
        {
            precision_ = precisionNeeded;

            // Update the time formatting
            setTime(startTime_, 0);

            WarningIn("Time::setControls()")
                << "Increasing the timePrecision from " << oldPrecision
                << " to " << precision_
                << " to support the formatting of the current time directory "
                << timeName() << nl << endl;
        }
        else
        {
            // Could not find time directory so assume it is not present
            precision_ = oldPrecision;

            // Revert the time formatting
            setTime(startTime_, 0);
        }
    }

henry

2015-03-08 12:30

manager   ~0004023

> it was missing a setTime in the last if block:

I noticed that and put it in.

wyldckat

2015-03-08 14:53

updater   ~0004024

Last edited: 2015-03-08 14:54

OK, on this topic, I think I can still do today some additional tests for the two fixes that you already have got scheduled to be committed, if you want to push them first to "OpenFOAM-dev".

-----

The 3rd and last topic I had in mind related to this was to make it "crystal clear" to the user from which time step the mesh is coming from... but it seems to require some considerable tweaking to the constructors of class "polyMesh".

The main tweak would be to enforce that the core files for the mesh ("faces", "points", etc...) should all come from the same time folder... which would remove the feature to have minimal changes to the mesh, e.g. be able to only change the "points" file of the mesh.

My guess here is that either a debug flag could be put in place for reporting these small details or use a function object for reporting where the current mesh files came from.
If you prefer/concur, I can create said function object.

henry

2015-03-08 15:03

manager   ~0004025

I am still running tests on the various changes to OpenFOAM-dev and I haven't yet completed the changes corresponding to patch-1 in a way I am entirely happy with. I hope to get this done by the end of the day.

> enforce that the core files for the mesh ("faces", "points", etc...) should all come from the same time folder...

I don't agree with this change.

> which would remove the feature to have minimal changes to the mesh, e.g. be able to only change the "points" file of the mesh

This is a very useful feature which we want to keep.

I am not convinced of any need to change the way the mesh is constructed or reported, I think it will introduce more complexity requiring maintenance. If you created a functionObject for this do you think users will actually use it?

wyldckat

2015-03-08 15:17

updater   ~0004026

> I hope to get this done by the end of the day.

OK, then I'll do some more tests later on and report back with another bug report, if the need arises.


> If you created a functionObject for this do you think users will actually use it?

Good point. Advanced users might use it, but many of those advanced users can create and use scripts to pinpoint where those files are coming from. Really advanced users simply know where their meshes are located!
Any other users would only use it if they see it in the Doxygen documentation and/or if they see it somewhere already explaining how to use it.

Very well, if the need ever arises with a good enough reason, this can be easily done at that time.
In fact, this is one those features simple enough to be used for training users in the art of creating function objects :) Or even with "codedFunctionObject"!

henry

2015-03-08 15:55

manager   ~0004027

I am playing with patch-1 still and wondering if sqrt(SMALL) is the appropriate tolerance. It doesn't work with SMALL; maybe it should relate to some fraction of deltaT_. What is the rationale for sqrt(SMALL)? Do you think this will be robust?

henry

2015-03-08 16:17

manager   ~0004029

How about 10*SMALL? My feeling is that sqrt(SMALL) will cause problems for cases with very small time-steps.

wyldckat

2015-03-08 16:31

updater   ~0004030

Sorry, I should have detailed this beforehand.
I originally was using "mag(deltaT*1.001)" vs "mag(deltaT)", but it felt like too much of a hard-coded hack, hence switching to something based on "SMALL".

The logic behind "sqrt(SMALL)" is that an error of roughly 1e-7 for something that should be "0.0" felt like it was "good enough". But a margin of error of "deltaT*1.001" is probably the easiest/best approximation.

Since the idea is to confirm that the added "deltaT" value worked as intended; the problem is that the margin of error should be near machine epsilon, but the addition of errors in the operations makes it something that could be in the order of 10 to 1000 times larger than machine epsilon, due to scalar->word->scalar operations :(

henry

2015-03-08 16:35

manager   ~0004031

This tolerance certainly relates to machine precision but my test show the issue is simply to do with the subtractions not the scalar->word->scalar operations. I have just pushed the changes with 100*SMALL as the tolerance. See if you can break it :-)

wyldckat

2015-03-08 16:36

updater   ~0004032

The check for "this still needs to be fixed" was:

  mag(rereadTimeValue - oldTimeValue) > mag(deltaT_*1.001)

But then in v1 I provided the "simpler" form:

  mag(rereadTimeValue - oldTimeValue - deltaT_) > sqrt(SMALL)

If the margin of error is in the same scale as "deltaT_" itself, then things are seriously not working as intended...


Forgot to also say that "10*SMALL" didn't work for me.

wyldckat

2015-03-08 17:14

updater   ~0004038

Last edited: 2015-03-08 17:14

> See if you can break it

Broke it twice in the same run :D (dev commit 30bbd3e9)

    Time = 99.96125

    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1123
        Increased the timePrecision from 7 to 15 to distinguish between timeNames at time 99.9613000000001
    Time = 99.9613000000001

    ...

    Time = 99.9999500000014

    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1123
        Increased the timePrecision from 15 to 16 to distinguish between timeNames at time 100.0000000000014
    Time = 100.0000000000014

henry

2015-03-08 17:57

manager   ~0004041

What is the smallest tolerance for which the case runs?

wyldckat

2015-03-08 18:27

updater   ~0004042

In this test case, it seems "10000*SMALL" is safe enough.
I'll do some more tests, to try and isolate what could be the origin of this issue...

henry

2015-03-08 18:32

manager   ~0004043

For DP 10000*SMALL would probably be OK but in SP it would be nonsense. I think we do need to find out why it is accumulating SOOO much error.

wyldckat

2015-03-08 19:07

updater   ~0004044

Last edited: 2015-03-08 19:09

I'm looking into the issue regarding the error level... it's weird what I'm seeing right now...

edit: Sorry... the weird detail is that it seemed that "deltaT" didn't even add up to "value()"...

wyldckat

2015-03-08 20:01

updater  

issue815.patch.v3 (2,932 bytes)

wyldckat

2015-03-08 20:14

updater   ~0004046

OK, attached is the file "issue815.patch.v3", which is relative to OpenFOAM-dev commit 30bbd3e9be9d.

The changes are (in order):

 1- Initialized "timeNameValue", because nothing worse than using memory garbage in case the "readScalar" fails due to some crazy reason... although I didn't check of "readScalar" already does this...

 2- "errorMargin" is a new variable that has the calculation that takes into account:
     a) the current expected numerical precision;
     b) the "deltaT" value;
     c) and the maximum allowed precision (i.e. SMALL).
    
    This is because the minimum precision needed should be the one currently being used, along with the current "deltaT"... and because we cannot allow it to go over machine epsilon, we should limit the expected numerical difference.

 3- "do-while" loop has a massive flaw of always increasing the precision. My tests could reach over 7700 precision... So I replaced with a "for" loop.

    ... Then again, not sure if this was the best solution... probably should have used "if(precision_<maxPrecision_)" for wrapping the "do-while" loop...

 4- With the replacement of the "do-while" loop for the "for" loop, the warning should only be issued if the precision has actually changed.

henry

2015-03-08 20:37

manager   ~0004047

1. I took out the initialization because in case the read fails false is returned and the value is not used. How does initializing to -VGREAT in particular help? Why not 0 or SMALL or anything else?

3. Given that we are looping to a maximum I prefer a for-loop anyway as I used in the code added to setControls.

I will merge in and test.

henry

2015-03-08 20:42

manager   ~0004048

Isn't
        for
        (
          /* no init */ ;
            precision_ < maxPrecision_
         && readScalar(dimensionedScalar::name().c_str(), timeNameValue)
         && (mag(timeNameValue - oldTimeValue - deltaT_) > errorMargin) ;
          /* no iter */
        )
simply a while-loop?

wyldckat

2015-03-08 20:56

updater   ~0004051

The "for" loop by concept will always check the middle statement at the start of the loop. The "while" loop.... Oh, good point! Yes it is!

Completely forgot that "while" loop exists in C/C++... I was assuming that "do-while" was the only one similar loop...


My experience in coding is that initializing values is always a must, since we never know for certain who's going to be messing with the code next; or even the original coder can forget how the variable was used in the first place. And the usual result is having to track down a bug that only occurs in release mode, because of uninitialised memory.
I chose "-VGREAT" since it would give the biggest difference in 99.99999% of the cases... except for negative pseudo-temporal time values (e.g. crank degree?).

henry

2015-03-08 21:10

manager   ~0004055

Just pushed the latest...

wyldckat

2015-03-08 21:27

updater   ~0004056

Tested it just now. OK, this is.... irgh...

Using the test package "runTimeSteps.tar.gz" I provided before, if we run the test case until its end, it will write the last step at "99.958".

Now, if we up the "deltaT" from "5.0e-4" to "0.5432112345e-5", the current algorithm is not able to provide a fully accurate precision right off the bat:

    Create mesh for time = 99.958

    ...
        Increased the timePrecision from 5 to 8 to distinguish between timeNames at time 99.958005
    Time = 99.958005

    ...
        Increased the timePrecision from 8 to 10 to distinguish between timeNames at time 99.95801086
    Time = 99.95801086

    ...
        Increased the timePrecision from 10 to 12 to distinguish between timeNames at time 99.9580162963
    Time = 99.9580162963

    ...
        Increased the timePrecision from 12 to 13 to distinguish between timeNames at time 99.95802172845
    Time = 99.95802172845

    ...
        Increased the timePrecision from 13 to 15 to distinguish between timeNames at time 99.9580271605617
    Time = 99.9580271605617

    ...
        Increased the timePrecision from 15 to 16 to distinguish between timeNames at time 99.95803259267407
    Time = 99.95803259267407

Then later on:

    Time = 99.99999566054473

    ...
        Increased the timePrecision from 16 to 17 to distinguish between timeNames at time 100.00000109265707
    Time = 100.00000109265707

    ExecutionTime = 0.41 s ClockTime = 0 s


This is because the "timeTol" is not updated whenever the precision is increased as well. Problem is that when I tried this, it didn't go so well.

In addition, I'm not able to do any more tests today, sorry :( With luck, I can do some tomorrow at the end of the day.

wyldckat

2015-03-14 14:04

updater   ~0004120

OK, I've finally managed to get a better look into this, but unfortunately when I reach Saturdays, my head doesn't run close enough to "optimal performance", therefore I can't figure out the "optimum solution"...

Anyway, what I can figure out is this:

 1- The current implementation seems to cover most of the cases, although not all of them. It progressively increases precision, in an attempt to not go "overkill" at the first confirmation that the precision needs updating.

    The only "blind spot" I can think of, with the current solution, is that the rounding algorithm can lead to a corner case situation conceptually similar to this one:

      Time step: 0.123456
      Delta time step: 0.0000005

      Time step: 0.1234565
      Delta time step: 0.000000048 (adapted from CourantNo)

      Time step: 0.1234566 (real 0.123456548)
      Delta time step: 0.000000008 (adapted from CourantNo)

      Time step: 0.123456556 (real 0.123456556)

    This isn't very realistic, since this seems to be covered by the min-max with "deltaT" and "pow(10,-precision)"... but it's the concept that matters here, namely the situation where a round-off could potentially break the time flow.

    I think that for now, this can be at least automatically detected and issue a warning to the user... at least for the current development. The idea is to do a check on whether the saved time folder didn't go in the wrong direction. If I'm not 100% certain about this, but from my tests, this seems to work as intended:

            //Check if there was a round-off glitch that reversed time
            scalar oldTimeNameValue = -VGREAT;
            if
            (
              readScalar(oldTimeName.c_str(), oldTimeNameValue) &&
              (sign(timeNameValue - oldTimeNameValue) != sign(deltaT_))
            )
            {
                WarningIn("Time::operator++()")
                    << "Current time name " << dimensionedScalar::name()
                    << " is set to an instance prior to the previous one "
                    << oldTimeName << endl
                    << " This might result in temporal discontinuities."
                    << endl;
            }

    This would theoretically circumvent all of the corner cases, at least the ones I can think of :(


 2- The next issue is regarding the recovery of a time snapshot that was saved with "enhanced precision". I'm referring to the "value" entry stored in the "uniform/time" file, for example in "99.9580001/uniform/time.value". Said "value" doesn't save with the same precision as the time folder, which according to the comments in the methods that use this value, seems to be the desired effect.

    The suggestion here would be to have at least one accurate way of storing the time data in binary format. This way it would be possible to reconstruct the time folders accurately, if the need ever arises. Said time reconstruction feature could be introduced in "foamListTimes" as an additional option... or at the very least, display the actual time snapshots each folder is referring to, so that the user fixes manually said folders.

henry

2015-03-14 14:33

manager   ~0004121

The check you added for issue 1 is not clear to me yet, I need to think about and test it. Where did you insert the test for change in time direction?

Issue 2 is clear and my feeling is that the time value should be stored in uniform/time with the same precision as the time directory i.e. we should write the timeName as a value. The alternatives would be to write it binary (I don't like this one at all) or write it ASCII but with sufficient precision to provide an accurate read. I think writing timeName as the value would be a good approach and I can't immediately see any issues with this.

wyldckat

2015-03-14 14:42

updater   ~0004123

Sorry, completely forgot about where to place the check for #1... I think it should be tested after this if-block in the method "Foam::Time::operator++()":

            if (precision_ == maxPrecision_)
            {
                // Reached maxPrecision limit
                WarningIn("Time::operator++()")
                    << "Current time name " << dimensionedScalar::name()
                    << " is the old as the previous one " << oldTimeName
                    << endl
                    << " This might result in overwriting old results."
                    << endl;
            }


As for your solution to #2: seems like a good solution. Precision "log10(SMALL)+1" should ensure the value to be properly stored in ASCII format; the extra digit would be an attempt to ensure nothing is missing from the number, in comparison to binary.

henry

2015-03-14 20:36

manager   ~0004127

I have just pushed these two changes to OpenFOAM-dev

henry

2015-03-15 10:19

manager   ~0004130

I am finding the new precision alteration code unreliable for cases with adjustable time-step, for example running

tutorials/multiphase/twoPhaseEulerFoam/laminar/mixerVessel2D

I get time directories;

3.9
4
4.1000000000000005
4.2000000000000002
4.2999999999999998
4.4000000000000004
4.5

wyldckat

2015-03-15 12:22

updater   ~0004131

Sorry, I found about this late last night. I didn't report right away, because I wanted to figure out first what's going wrong :(

I found out when I re-ran my test case yesterday from "0", instead from 99.958 and found the same symptom, for example:

- Settings:

    startTime 0;
    stopAt endTime;
    endTime 100;
    deltaT 0.5e-5;
    writeControl runTime;
    writeInterval 0.54325;
    purgeWrite 0;
    writeFormat ascii;
    writePrecision 6;
    writeCompression off;
    timeFormat general;
    timePrecision 2;

- Snippet of the first major problem:

    Time = 49.146475

    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1127
        Increased the timePrecision from 8 to 10 to distinguish between timeNames at time 49.14648001
    Time = 49.14648001


Unfortunately, I won't be able to diagnose this in more detail today. Nonetheless, I expect I'll be able to do so next weekend. Sooner if I can deduce what's going on.


Nonetheless, from the numbers I checked yesterday from comment #4056, it looked like one of the critical problems was the reliance on the updated precision, even though it's only done at the next "deltaT" update. For example, when the precision went from 1e-8 to 1e-10, it was already overkill, although it was necessary due to the difference in "t2-t1-dT" being at a scale of "Xe-9" that was larger than "1e-9".
At the time I simply accepted it as a "good thing" and was why I reported on comment #4120: "The current implementation seems to cover most of the cases [...]"


This feels like something that is already solved somewhere in the programming scientific community, but I haven't managed to look for it yet either. I'll let you know if I find out something in the meantime.

henry

2015-03-15 18:56

manager   ~0004135

I have just pushed this change:

commit 7dfdea2cac3c955a174168d3d7671a57cc3ece19
Author: Henry <Henry>
Date: Sun Mar 15 18:49:52 2015 +0000

    Time: Adjust the precision of the time-directories only for write-times
    Avoids unnecessary increases in precision during intermediate steps for
    cases with adjustable time-step which may require very high precision to
    represent the time name.

My feeling is that now that the precision adjustment guarantees sufficient precision irrespective of the time and time-step, for cases in which the time-step is adjusted this precision adjustment can ONLY be applied for output times otherwise it will rapidly increase to the upper limit.

The only issue if for cases in which intermediate results are written explicitly for any time. For such cases the precision must be set by the user appropriately.

wyldckat

2015-03-22 00:20

updater   ~0004181

I've finished doing a few more tests with the latest commit and it does seem to work very well!
Or at the very least, it won't reveal the issues we've seen during the development of this feature, unless the user is writing every single time step, which is unlikely in a non-debug-type simulation... in which situation of a debug, the user should have increased the precision to maximum before even starting the run :)

Although it does look weird, the solver showing several identical time steps... but it's the user's fault anyway :)


Nonetheless, I was able to trigger a curious bug/detail... here's the message I got:

    Time = 128.20623875389433

    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1302
        Increased the timePrecision from 17 to 18 to distinguish between timeNames at time 128.206781965128812
    --> FOAM Warning :
        From function Time::operator++()
        in file db/Time/Time.C at line 1312
        Current time name 128.206781965128812 is the old as the previous one 128.20623875389433
        This might result in overwriting old results.
    Time = 128.206781965128812

So essentially the problem is the second warning, which is triggered when "precision_ == maxPrecision_", which reveals two issues:

 1- The sentence:

      "Current time name ... is the old as the previous one ..."

    sounds strange. Maybe it was meant to be:

      "Current time name ... is the same as the previous one ..."

 2- Uh... assuming the correction is right, then the sentence isn't a valid statement anyway, since the values are clearly different. It seems to me that the correct statement would be something like:

      "The maximum time precision has been reached. This is the last warning regarding time precision you'll see during this run."

    In which case, the statement below that is still valid:

      " This might result in overwriting old results."


Beyond this last textual adjustment, I think it's good to go! We've already covered so many situations, that the last resort would be to either go Quad Precision or go "binary" for those who want perfect time precision, in which case, it would probably require a more dedicated data storage handling mechanism.

henry

2015-03-22 09:52

manager   ~0004183

I think we have reached a point where users can test the new functionality in anger and provide feedback.

I have changed the max-precision messages as you suggested.

Resolved by commit fbb0a87995dc41c2183a1bc2648790352902d677 in OpenFOAM-dev

Issue History

Date Modified Username Field Change
2013-04-15 08:32 projectionist New Issue
2013-04-15 08:32 projectionist File Added: controlDict
2013-07-07 19:05 user258 Note Added: 0002310
2013-07-08 10:04 user4 Note Added: 0002311
2015-03-06 14:22 wyldckat Note Added: 0003992
2015-03-07 19:58 wyldckat File Added: runTimeSteps.tar.gz
2015-03-07 20:12 wyldckat Note Added: 0004003
2015-03-07 21:35 wyldckat File Added: Time.C.v1
2015-03-07 21:35 wyldckat File Added: issue815.patch.v1
2015-03-07 21:47 wyldckat Note Added: 0004004
2015-03-07 22:02 henry Note Added: 0004006
2015-03-07 22:20 wyldckat Note Added: 0004007
2015-03-07 22:43 wyldckat Note Added: 0004008
2015-03-07 22:43 wyldckat File Added: issue815.patch.v2
2015-03-07 23:53 henry Note Added: 0004010
2015-03-08 10:50 henry Note Added: 0004011
2015-03-08 11:42 wyldckat Note Added: 0004012
2015-03-08 11:51 henry Note Added: 0004013
2015-03-08 11:52 wyldckat Note Added: 0004014
2015-03-08 11:57 henry Note Added: 0004015
2015-03-08 12:04 wyldckat Note Added: 0004016
2015-03-08 12:07 henry Note Added: 0004017
2015-03-08 12:11 wyldckat Note Added: 0004018
2015-03-08 12:14 henry Note Added: 0004019
2015-03-08 12:21 wyldckat Note Added: 0004020
2015-03-08 12:22 wyldckat Note Edited: 0004020
2015-03-08 12:24 henry Note Added: 0004021
2015-03-08 12:27 wyldckat Note Added: 0004022
2015-03-08 12:30 henry Note Added: 0004023
2015-03-08 14:53 wyldckat Note Added: 0004024
2015-03-08 14:54 wyldckat Note Edited: 0004024
2015-03-08 15:03 henry Note Added: 0004025
2015-03-08 15:17 wyldckat Note Added: 0004026
2015-03-08 15:55 henry Note Added: 0004027
2015-03-08 16:17 henry Note Added: 0004029
2015-03-08 16:31 wyldckat Note Added: 0004030
2015-03-08 16:35 henry Note Added: 0004031
2015-03-08 16:36 wyldckat Note Added: 0004032
2015-03-08 17:14 wyldckat Note Added: 0004038
2015-03-08 17:14 wyldckat Note Edited: 0004038
2015-03-08 17:57 henry Note Added: 0004041
2015-03-08 18:27 wyldckat Note Added: 0004042
2015-03-08 18:32 henry Note Added: 0004043
2015-03-08 19:07 wyldckat Note Added: 0004044
2015-03-08 19:09 wyldckat Note Edited: 0004044
2015-03-08 20:01 wyldckat File Added: issue815.patch.v3
2015-03-08 20:14 wyldckat Note Added: 0004046
2015-03-08 20:37 henry Note Added: 0004047
2015-03-08 20:42 henry Note Added: 0004048
2015-03-08 20:56 wyldckat Note Added: 0004051
2015-03-08 21:10 henry Note Added: 0004055
2015-03-08 21:27 wyldckat Note Added: 0004056
2015-03-14 14:04 wyldckat Note Added: 0004120
2015-03-14 14:33 henry Note Added: 0004121
2015-03-14 14:42 wyldckat Note Added: 0004123
2015-03-14 20:36 henry Note Added: 0004127
2015-03-15 10:19 henry Note Added: 0004130
2015-03-15 12:22 wyldckat Note Added: 0004131
2015-03-15 18:56 henry Note Added: 0004135
2015-03-22 00:20 wyldckat Note Added: 0004181
2015-03-22 09:52 henry Note Added: 0004183
2015-03-22 09:52 henry Status new => resolved
2015-03-22 09:52 henry Resolution open => fixed
2015-03-22 09:52 henry Assigned To => henry
2015-03-24 00:17 liuhuafei Issue cloned: 0001589