AutoPas icon indicating copy to clipboard operation
AutoPas copied to clipboard

Rethink `AutoTuner::willRebuildNeighborLists()`

Open FG-TUM opened this issue 2 years ago • 0 comments

Is your feature request related to a problem? Please describe. https://github.com/AutoPas/AutoPas/blob/3053eeca87758a9e89670c62fdb53a83cd8ea2e8/src/autopas/tuning/AutoTuner.cpp#L268-L275

AutoTuner::willRebuildNeighborLists() currently works by counting iterations and, from there, estimating if neighbor lists should be rebuilt because of the rebuild frequency or container changes due to tuning. The problem with this is, that if we ever change our logic, such that we rebuild earlier, e.g. by collecting fewer samples than _maxSamples, this logic breaks down producing silent errors.

Describe the solution you'd like A more robust estimation system that is not based on (too much) state.

Describe alternatives you've considered Sam's suggestion is to give the AutoTuner a flag for if we are switching configurations. This can easily be set within AutoTuner::getNextConfig(). We can remove all the rebuild-naturally-after-_rebuildFrequency-iterations logic from AutoTuner::willRebuildNeighborLists(). Then, all the function does is determine if the neighbour lists should be rebuilt due to changing configuration (and so should get renamed e.g. AutoTuner::willRebuildNeighborListsDueToChangingConfiguration()). And so all the function needs to do is return true or false depending on if the flag is set and, if so, reset the flag as well. Originally posted by @SamNewcome in https://github.com/AutoPas/AutoPas/pull/759#discussion_r1318276202

Additional context This logic was introduced after the AutoTuner lost access to the container in the process of creating #759

FG-TUM avatar Sep 14 '23 13:09 FG-TUM