Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re arrange read time according to adjustable dt #298

Merged
merged 13 commits into from
Nov 28, 2023

Conversation

davidscn
Copy link
Member

@davidscn davidscn commented Jul 27, 2023

Only the last commit is relevant

Since read data is now affiliated to a time stamp, we need to ensure that the time stamp we read data is consistent with our solver time-step size. However, the solver time-step size changes in different places, depending on the configuration in the controlDict.

  • If we have a non-adjustable/fixed time-step size in our OpenFOAM solver, the time-step size adjustment is triggered in the execute routine of the functionObject. We have the order
functionObject::configure{
...
  dtN = adjustDt()
  readData(dtN)
}

functionObject::execute{
  writeData()
  advance(dtN)
  [readCheckpoint]
  [writeCheckpoint]
  dtN1 = adjustDt()
  readData(dtN1)
}
  • If we have an adjustable time-step size in our OpenFOAM solver, the time-step size adjustment is triggered in the adjustTimeStep routine of the functionObject, which is triggered in the beginning of each time-step. This results in the order:
functionObject::execute{
  writeData()
  advance(dtN)
  [readCheckpoint]
  [writeCheckpoint]
}

// at the beginning of each (also the first) time step
functionObject::adjustTimeStep{
  dtN1 = adjustDt()
  readData(dtN1)

The described data reading is consistent with a backward Euler scheme (or just ddt Euler in OpenFOAM terminology) and not with all the time-integration schemes OpenFOAM offers.

This PR ensures the readData logic according to the described scenarios.

The only thing (feature-wise), which still does not work with the current workflow is the writeControl adjustable; option and only the option writeControl runTime; works as expected. This option only affects the generation of result files, though. However, I think this was already the case before we introduced the changes here, correct @MakisH ? I'm still a bit confused, where and how the first option modifies the time-step size (triggered actually precice/precice#1751 ), but I guess we cannot do a whole lot about it.

DB driven development

TODO list:

  • I updated the documentation in docs/
  • I added a changelog entry in changelog-entries/ (create directory if missing)

@MakisH MakisH self-requested a review August 2, 2023 07:53
@MakisH
Copy link
Member

MakisH commented Aug 3, 2023

The only thing (feature-wise), which still does not work with the current workflow is the writeControl adjustable; option and only the option writeControl runTime; works as expected. This option only affects the generation of result files, though. However, I think this was already the case before we introduced the changes here, correct @MakisH ? I'm still a bit confused, where and how the first option modifies the time-step size (triggered actually precice/precice#1751 ), but I guess we cannot do a whole lot about it.

Here is the respective OpenFOAM code:

void Foam::Time::adjustDeltaT()
 {
     bool adjustTime = false;
     scalar timeToNextWrite = VGREAT;
  
     if (writeControl_ == wcAdjustableRunTime)
     {
         adjustTime = true;
         timeToNextWrite = max
         (
             0.0,
             (writeTimeIndex_ + 1)*writeInterval_ - (value() - startTime_)
         );
     }
  
     if (adjustTime)
     {
         scalar nSteps = timeToNextWrite/deltaT_;
  
         // For tiny deltaT the label can overflow!
         if (nSteps < scalar(labelMax))
         {
             // nSteps can be < 1 so make sure at least 1
             label nStepsToNextWrite = max(1, round(nSteps));
  
             scalar newDeltaT = timeToNextWrite/nStepsToNextWrite;
  
             // Control the increase of the time step to within a factor of 2
             // and the decrease within a factor of 5.
             if (newDeltaT >= deltaT_)
             {
                 deltaT_ = min(newDeltaT, 2.0*deltaT_);
             }
             else
             {
                 deltaT_ = max(newDeltaT, 0.2*deltaT_);
             }
         }
     }
  
     functionObjects_.adjustTimeStep();
 }

writeControl adjustable sets the timeToNextWrite, which then affects deltaT_. deltaT_ is a member variable of the TimeState class. This is used to compute the time step size in the next iteration:

if (adjustTime)
     {
         scalar nSteps = timeToNextWrite/deltaT_;

I think that you could test the behavior by specifying a fixed deltaT that is not perfectly dividing writeInterval (e.g., 0.3 and 1.0). You should end up with the directories 0/ 0.3/ 0.6/ 0.9/ 1.0/.

preCICE does something similar, so the two mechanisms compete. I don't find any particular documentation (code/thesis) regarding writeControl in the adapter. While I remember being puzzled with all the different possibilities, I don't remember this particular aspect being an unsolved problem (maybe I faced an issue that I later fixed). In case there are issues with that, we should better check and throw an error, as this is rather a common setting.

I am struggling a bit to evaluate all paths, but I think the changes make sense, especially since now the read_data() is coupled to the dt.

Questions:

Are you sure that the dt we are determining everywhere is the one matching the read_data()? I am always a bit confused with what is used now, what is used next, and what preCICE needs.

How can we test that the changes still give the same (or more correct) results? What have you checked and what is still open?

Other than these questions, I think that your contribution is on the right rails.

@davidscn
Copy link
Member Author

davidscn commented Aug 4, 2023

Main reason why the writeControl setting is problematic now is that quite some assertions were added during waveform implementations (precice/precice#1751). Maybe it would be a good idea to wait until the mentioned issue is resolved, because the assertion messages I get from preCICE don't help to figure out apparent issues. I'm also wondering whether all the assertions in preCICE consider rounding errors and were tested using real solvers. In principle, running everything using release builds works without any error message from preCICE.

@MakisH MakisH mentioned this pull request Aug 4, 2023
2 tasks
@MakisH
Copy link
Member

MakisH commented Aug 4, 2023

So, status: Looks good, but waiting for #285 and #297 to be merged (and then rebase this branch) and for precice/precice#1751 to be resolved in the library. We will potentially need to document any restrictions regarding writeControl.

@MakisH
Copy link
Member

MakisH commented Aug 7, 2023

Merged all dependencies into develop and merged develop back to this branch.
Also added a changelog entry.

Next step: waiting for precice/precice#1751 to be resolved in the library. We will potentially need to document any restrictions regarding writeControl.

@MakisH
Copy link
Member

MakisH commented Nov 21, 2023

Update: I triggered precice/precice#1890 while testing the adapter from develop. However, even when using this branch, I run into the same issue when subcycling. We should wait for that to be fixed.

See also precice/tutorials#406, which uses subcycling.

Copy link
Member

@MakisH MakisH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All known related issues seem to be fixed, and we now have a case with subcycling that works: precice/tutorials#406

This looks good to go! 🚀 👌

@MakisH MakisH merged commit 99421f7 into precice:develop Nov 28, 2023
@davidscn davidscn deleted the re-arrange-read branch November 28, 2023 09:41
This was referenced Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants