Add LTS Wiggle Factor #758
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #758 +/- ##
==========================================
+ Coverage 10.18% 15.52% +5.34%
==========================================
Files 262 291 +29
Lines 18189 21759 +3570
==========================================
+ Hits 1852 3378 +1526
- Misses 16337 18381 +2044
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The clustering changes with the wiggle factor because the time step width of the clusters change |
krenzland
force-pushed
the
lts-wiggle-factor
branch
from
55a753f
to
35d51eb
Compare
Add missing file
But do not use it yet for output
Not 100% working but better than before
krenzland
force-pushed
the
lts-wiggle-factor
branch
from
ecbd430
to
2cdd410
Compare
EnforceMaximumDifference must be called before merging groups to get accurate cost estimate
Also fix small bug that sometimes created empty buffers when not merging clusters.
There was a problem hiding this comment.
Hi, the code looks good to me, most of my comments are about the term group, which is misleading IMHO.
Also, I think that the setup is too complicated to document it with the parameter file only. There are, in fact, two features: The wiggle factor and the maximum number of allowed clusters (auto merge). So please add some more documentation in its own RtD chapter.
|
|
||
| // Compute baseline cost. No wiggle, no group merge | ||
| m_clusterIds = computeClusterIds(maxWiggleFactor); | ||
| totalWiggleFactorReductions += enforceMaximumDifference(); |
There was a problem hiding this comment.
I would change to (see also comment above):
if (ltsParameters->getWiggleFactorEnforceMaximumDifference()) { totalWiggleFactorReductions += enforceMaximumDifference();}
There was a problem hiding this comment.
I m sorry I don't understand why you resolved
| // performance. | ||
| bool foundAdmissibleMerge = false; | ||
| for (const auto& [noOfGroups, cost] : mapNoOfGroupsToLowestCost) { | ||
| if (cost <= maxAdmissibleCost) { |
There was a problem hiding this comment.
Do I understand correctly that you would choose the wiggle factor leading to the lowest number of clusters rather than the lowest cost?
I think it would make more sense to compute maxAdmissibleCost based on the lowest cost (e.g. lowest*1.2), and then choose from all wiggle factor the one with the lowest number of clusters.
Then you won't miss a wiggle factor with very low costs but many clusters.
There was a problem hiding this comment.
I mean, if you gain 0.2% with the wiggle factor and then allow for 10%, then the wiggle factor appears useless in most cases.
| const auto should = 0; | ||
| for (int i = 1; i <= 5; ++i) { | ||
| const auto is = computeMaxClusterIdAfterAutoMerge( | ||
| enforceMaxClusterId(clusterIds, 0), cellCosts, 1, i, 0, 0); |
There was a problem hiding this comment.
I would remove this enforceMaxClusterId and put should=2
|
|
||
| // Compute baseline cost. No wiggle, no group merge | ||
| m_clusterIds = computeClusterIds(maxWiggleFactor); | ||
| totalWiggleFactorReductions += enforceMaximumDifference(); |
There was a problem hiding this comment.
I m sorry I don't understand why you resolved
| auto maxClusterIdToEnforce = ltsParameters->getMaxNumberOfClusters() - 1; | ||
| if (ltsParameters->isAutoMergeUsed()) { | ||
| const auto maxClusterIdAfterMerging = | ||
| computeMaxClusterIdAfterAutoMerge(m_clusterIds, |
There was a problem hiding this comment.
if the maxAdmissibleCost does not depend on the wiggle, then why not applying auto merge after you found the best wiggle?
|
With the most recent commit, this PR should now be bug free™. It at least seems to work for a production scenario of mine. |
Documentation/local-timestepping.rst
Outdated
| SeisSol enforces some constraints on the clustering, for example, neighboring elements are always either in the same cluster, | ||
| or a cluster which has at most a time step size difference of 2. | ||
| Elements that are connected by a dynamic rupture face have to be in the same time-cluster. | ||
| THis is called the maximum difference property. |
Documentation/local-timestepping.rst
Outdated
| To enable it, use the setting :code:`ClusteredLTS = 2`. | ||
| To disable it, use the value :code:`ClusteredLTS = 1` |
Documentation/local-timestepping.rst
Outdated
| ---------------------------- | ||
|
|
||
| SeisSol enforces some constraints on the clustering, for example, neighboring elements are always either in the same cluster, | ||
| or a cluster which has at most a time step size difference of 2. |
|
Thanks @sebwolf-de, I've fixed these issues. |
(Draft PR because currently NOT well tested!)
This PR implements the "wiggle factor" described in https://doi.ieeecomputersociety.org/10.1109/IPDPS53621.2022.00046
The idea is to make the time step of all clusters smaller. This can change the clustering and can move some elements towards a higher cluster - leading to a faster time-to-solution.
Note: Searching for this wiggle factor can be quite expensive. It can be made cheaper by setting "LtsWiggleFactorEnforceMaximumDifference = 0" but this may lead to a slightly worse wiggle factor.
(The default behavior is that this wiggle factor is NOT used because it can make unstable CFL constants seem stable.)