refactor: EnergyNode and SPCFw unit test#2326
Conversation
trisyoungs
force-pushed
the
dissolve2/energy-unit-tests
branch
from
edfa223 to
6547b83
Compare
trisyoungs
force-pushed
the
dissolve2/energy-unit-tests
branch
2 times, most recently
from
ca3e9e4 to
5d290f5
Compare
…c number of atoms is defined in the file.
trisyoungs
force-pushed
the
dissolve2/energy-unit-tests
branch
from
5d290f5 to
65e5f5c
Compare
rprospero
left a comment
rprospero
left a comment
There was a problem hiding this comment.
Looks good. Just included some thoughts that it inspired.
| // Update histories | ||
| totalEnergyHistory_.push(energy.pairPotential.total() + energy.geometry.total(), podHistoryLength_); | ||
| totalPairPotentialHistory_.push(energy.pairPotential.total(), podHistoryLength_); | ||
| totalMoleculePPHistory_.push(energy.pairPotential.intraMolecular, podHistoryLength_); | ||
| totalCohesiveHistory.push(energy.pairPotential.interMolecular, podHistoryLength_); | ||
| totalGeometryHistory_.push(energy.geometry.total(), podHistoryLength_); | ||
| bondHistory_.push(energy.geometry.bondEnergy, podHistoryLength_); | ||
| angleHistory_.push(energy.geometry.angleEnergy, podHistoryLength_); | ||
| torsionHistory_.push(energy.geometry.torsionEnergy, podHistoryLength_); | ||
| improperHistory_.push(energy.geometry.improperEnergy, podHistoryLength_); | ||
|
|
||
| // Determine stability of energy | ||
| // Check number of points already stored for the Configuration | ||
| auto grad = 0.0; | ||
| auto stable = false; | ||
| if (stabilityWindow_ > totalEnergyHistory_.history().size()) | ||
| message("Too few points to assess stability.\n"); | ||
| else | ||
| { | ||
| auto yMean = 0.0; | ||
| grad = Regression::linearGradient(totalEnergyHistory_.history(), stabilityWindow_, yMean); | ||
| auto thresholdValue = fabs(stabilityThreshold_ * yMean); | ||
| stable = fabs(grad) < thresholdValue; | ||
|
|
||
| message("Gradient of last {} points is {:e} kJ/mol/step (absolute threshold value is " | ||
| "{:e}, stable = {}).\n", | ||
| stabilityWindow_, grad, thresholdValue, DissolveSys::btoa(stable)); | ||
| } | ||
|
|
||
| targetConfiguration_->setEnergyGradient(grad); | ||
| targetConfiguration_->setEnergyStable(stable); |
There was a problem hiding this comment.
I don't think that we should change any of this code right now, but it has given me some thoughts.
First, I'm thinking about a history node. The idea being that the node has a single Number input, a name option, and a history output. That opens up a couple of options. The user could pass the history to a graph node and get a graph of the history in a separate tab. It would also be possible to gain access to the gradient and perhaps perform different actions based on it.
Second, I'm also wondering about a user customizable dashboard. Values, such as strings, Numbers, or histories, could be sent into sinks in the graph with a name option. Then, in a separate panel, this information could all be in one place as a custom display to watch the simulation progress.
Finally, I'm thinking that I need to do more work on inner graphs. If we went with my history node idea and broken up the energy node into multiple parts, it would be an annoying pain for the users to wire everything back up again for every input file. Instead, we'd likely want to provide a node which did all of this for them. More concretely, I believe that we want
- A folder with a library of common subgraphs
- The ability to insert a library subgraph as a node
- The ability to add these subgraphs to the regular "Insert" menu in the GUI
I feel like we'll need this inner graph part even if we don't head down the history or dashboard roads.
There was a problem hiding this comment.
Yes, absolutely - the dashboard road is basically what I had in mind from the beginning in my mind's eye, as either a side panel, a full (separate) overview panel, or as individual tiny readouts / graphs on the nodes themselves (perhaps as a sort of GraphViewType option, switching between the display of inputs / outputs / options and the readout data).
Whether this data presentation / access should be achieved via Nodes I'm not so sure. This adds another layer of effort for the user to achieve what should be (in my opinion) a one-click (menu) operation. We could instead consider the idea of tagging data internally as exportable / plottable / viewable in the Node constructor, and just presenting those as options where necessary.
2 participants
This PR makes a fairly significant refactoring of the
EnergyNode, simplifying its functions (along with those of the oldEnergyModule) and providing only calculation of test energies with simple loops. Only the SPC/Fw energy tests are reimplemented here since there is a strong relationship with theForcesModuleunit tests in terms of data. So, with the added bonus of not bloating the PR too much, other energy tests will be implemented onceForcesModuletesting is moved over.