feat: more control of averaging potentials #1970
Conversation
Danbr4d
added
Type: Enhancement
There was a problem hiding this comment.
For me, I think this PR is valuable as a proof of concept study that the averaging of potentials makes a positive difference, but we shouldn't merge it in in it's current form, simply because if we did we would immediately have to fix it up in a patch release anyway.
I think the best route forward is to introduce a PotentialSet class (distinct from a PartialSet and a PotentialMap, and living somewhere in the middle - we might consider renaming the latter to make the separation clear, since the new PotentialSet class would be based around a std::map) into which we can implement the necessary de/serialisation and += / /= operators necessary for the Averaging functions to work.
| @@ -35,6 +36,12 @@ class EPSRManagerModule : public Module | |||
| }; | |||
| // Potential scalings | |||
| std::string potentialScalings_; | |||
| // Number of historical partial sets to combine into final partials | |||
There was a problem hiding this comment.
| // Number of historical partial sets to combine into final partials | |
| // Number of historical partial sets to combine into final potentials |
| @@ -35,6 +36,12 @@ class EPSRManagerModule : public Module | |||
| }; | |||
| // Potential scalings | |||
| std::string potentialScalings_; | |||
| // Number of historical partial sets to combine into final partials | |||
| std::optional<int> averagingLength_; | |||
| // Weighting scheme to use when averaging partials | |||
There was a problem hiding this comment.
| // Weighting scheme to use when averaging partials | |
| // Weighting scheme to use when averaging potentials |
src/modules/epsrManager/process.cpp
Outdated
| // Check if ran the right amount of iterations before averaging | ||
| if (averagedPotentialsStore.size() > averagingLength_) | ||
| { | ||
| averagedPotentialsStore.pop_back(); | ||
| } |
There was a problem hiding this comment.
This won't account for cases where the user changes the averaging length and we need to remove more than one. This curation of data is also handled for us if we move to using the proper Averaging namespace functions.
src/modules/epsrManager/process.cpp
Outdated
| for (auto &&[key, epData] : pots) | ||
| { | ||
| averagedPotentials[key].ep += epData.ep; | ||
| averagedPotentials[key].ep /= averagingLength_.value(); |
There was a problem hiding this comment.
Careful - the value of averagingLength_ may very well not be equal to the number of potential sets in the averagedPotentialStore.
Danbr4d
force-pushed
the
epsr-averaging
branch
from
0e0b8e9
to
25a1d2f
Compare
Co-authored-by: Adam Washington <[email protected]>
Danbr4d
force-pushed
the
epsr-averaging
branch
from
3082f25
to
7f6629e
Compare
Added in capability to average over a specified number of iterations instead of each time. The hope is that this will fix an issue of spikes in r-factor when running the averaging of potentials currently.