Directly perform extrapolation in time::Storage, only allow CouplingScheme to moveToNextWindow#1639
Conversation
* Remove performReceiveOfFirstAdvance. * Implement specific exchangeInitialData in SerialCouplingScheme and ParallelCouplingScheme.
BenjaminRodenberg
left a comment
BenjaminRodenberg
left a comment
There was a problem hiding this comment.
I added a few comments on how we dealt with extrapolation previously and what we do now. Hopefully this clarifies the open point about UNDEFINED_EXTRAPOLATION_ORDER.
One important part that I did not touch in this PR, but that is relevant: An explicit coupling scheme must use UNDEFINED_EXTRAPOLATION_ORDER, while an implicit coupling scheme must use (_extrapolationOrder == 0) || (_extrapolationOrder == 1). See in BaseCouplingScheme.cpp:
if (isExplicitCouplingScheme()) {
PRECICE_ASSERT(_extrapolationOrder == UNDEFINED_EXTRAPOLATION_ORDER, "Extrapolation is not allowed for explicit coupling");
} else {
PRECICE_ASSERT(isImplicitCouplingScheme());
PRECICE_CHECK((_extrapolationOrder == 0) || (_extrapolationOrder == 1),
"Extrapolation order has to be 0 or 1.");
}
| PRECICE_TRACE(_timeWindows); | ||
| for (auto &data : getAccelerationData() | boost::adaptors::map_values) { | ||
| for (auto &data : _allData | boost::adaptors::map_values) { | ||
| data->moveToNextWindow(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Regarding extrapolation this is one important bit I changed: Before, we only called moveToNextWindow for the accelerated data. This applied the extrapolation. Now, we move _allData.
Especially for explicit coupling schemes this part changed a lot: Previously this function was never called for explicit coupling schemes, because extrapolation is not reasonable for explicit coupling schemes:
if (isImplicitCouplingScheme()) {
if (not doesFirstStep()) {
storeExtrapolationData();
moveToNextWindow();
}
}
Now, we always call it for all coupling schemes, because all coupling schemes have to move the storage and extrapolation is just an optional feature of moving.
| if (_couplingScheme->isTimeWindowComplete()) { | ||
| for (auto &context : _accessor->readDataContexts()) { | ||
| context.moveToNextWindow(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Also important regarding extrapolation: Previously we called moveToNextWindow for the storage of the ReadDataContext here. I moved this inside the coupling scheme, because then we don't have to worry about convergence etc. in ParticipantImpl anymore.
| void CouplingData::moveToNextWindow() | ||
| { | ||
| _extrapolation.moveToNextWindow(); | ||
| values() = _extrapolation.getInitialGuess(); | ||
|
|
||
| this->setSampleAtTime(time::Storage::WINDOW_END, sample()); | ||
| } | ||
|
|
||
| void CouplingData::storeExtrapolationData() | ||
| { | ||
| const auto stamples = this->stamples(); | ||
| PRECICE_ASSERT(stamples.size() > 0); | ||
| this->values() = stamples.back().sample.values; | ||
| _extrapolation.store(values()); | ||
| if (this->timeStepsStorage().stamples().size() > 0) { | ||
| this->timeStepsStorage().move(); | ||
| auto atEnd = this->timeStepsStorage().stamples().back(); | ||
| PRECICE_ASSERT(math::equals(atEnd.timestamp, time::Storage::WINDOW_END)); | ||
| _data->sample() = atEnd.sample; | ||
| } | ||
| } |
There was a problem hiding this comment.
Previously, this did not move the Storage of the coupling data, but computed the extrapolation (remember: for acceleration data) and stored it at the WINDOW_END of the Storage. Technically this is the same like moving, but the implementation is different.
BenjaminRodenberg
changed the title
|
1c676d6 is interesting: I think I found a fix, but at the moment this test also fails on develop. Meaning: There is probably a bug with respect to this combination of features on develop. I will try to understand this better on develop by using this test there tomorrow. |
|
As directly discussed with @uekerman: This PR is ready to go and I will merge as soon as the tests have completed. There are still some details that we did not 100% finalize yet, but this will change and we will reiterate on these details soon again when subcycling + higher interpolation is implemented. |
* Add tests to reproduce faulty behavior. * Use initialCommunication as it has already been introduced in #1639.
* Add tests to reproduce faulty behavior. * Dirty fix. Needs refactoring. * Refactoring. * Cleanup comments. * Reduce diff. * Naming: Use initialCommunication as introduced in #1639. * Make substeps=false default. * Use substeps=true only in tests where it is necessary * Mark buggy tests for substeps=false (Compositional Coupling Scheme) * Remove unnecessary exception for compositional coupling scheme. Works now. Not sure why. * Use substeps=true. * Update changelog. * Update docs/changelog/1664.md --------- Co-authored-by: Benjamin Uekermann <[email protected]>
2 participants
Main changes of this PR
Replaces #1520 and #1504. Extrapolation now happens directly inside Storage and Storage is directly maintained in CouplingData.
Motivation and additional information
ExtrapolationclassreceiveResultOfFirstAdvancefromParticipantImplCouplingSchemeviaCouplingDataCouplingSchemetakes care ofmoveToNextWindow(previously also partially done byParticipantImpl)Author's checklist
pre-commithook to prevent dirty commits and usedpre-commit run --allto format old commits.I added a changelog file with(not needed, basically refactoring)make changelogif there are user-observable changes since the last release.Reviewers' checklist