Hamiltonian updates for batched estimators #5295
Conversation
PDoakORNL
force-pushed
the
HamiltonianUpdatesForBatchedEstimators
branch
from
a8dff12 to
3f1ccb5
Compare
src/QMCHamiltonians/CoulombPBCAA.cpp
Outdated
| @@ -317,6 +318,30 @@ void CoulombPBCAA::mw_evaluatePerParticle(const RefVectorWithLeader<OperatorBase | |||
|
|
|||
| for (const ListenerVector<RealType>& listener : listeners) | |||
| listener.report(walker_index, name, v_sample); | |||
|
|
|||
| #if defined TRACE_CHECK && defined NDEBUG | |||
There was a problem hiding this comment.
I'm afraid the if defined protected code is likely to become dead code.
How about we introduce trace_check=yes/no input option for this class and similar ones?
Also throw proper exception instead using the ugly APP_ABORT.
We can turn on trace_check in the tests although not by default.
There was a problem hiding this comment.
Not an unreasonable fear. I will take a look at doing that tomorrow morning, otherwise I will remove.
There was a problem hiding this comment.
I prefer no more additional options if we can avoid them. Can we enable TRACE_CHECK purely in debug builds? Currently TRACE_CHECK is never activated in any CI or nightly test builds, nor featured in any documentation, so the check almost doesn't exist. If the code is broken the check will fail every Monte Carlo step.
(Comments apply to most of the extra preprocessor enabled checks in the code -- I think we should be getting rid of them as separate options as much as we can and simply making them active in debug builds so that they are actually used.)
There was a problem hiding this comment.
This APP_ABORT came with the copy paste of the trace block and dates to 2013 or before when I don't think there was any other guidance on this. I've fixed it for the added code. There are many other APP_ABORT throughout the QMCHamiltonian module many in within the scope of a QMCSection, they seem as if they will not be caught until qmcapp.cpp which is where the handling is for most non uniform runtime errors. But some significant percentage of them them are predictable at construction time and actually uniform and are just eating some branch prediction capability. I'll write an issue for them.
There was a problem hiding this comment.
When code gets touched, the new code should follow our dev spec. https://qmcpack.readthedocs.io/en/develop/developing.html#log-and-error-output
There was a problem hiding this comment.
I made this just protected by a
#ifndef NDEBUG
blocck so its not dead code and in debug it always runs, since this doesn't create new input it seems to have an insignificant impact on debug "performance" so far. The other "trace" code in that estimator needs more thought to make not "dead".
PDoakORNL
force-pushed
the
HamiltonianUpdatesForBatchedEstimators
branch
from
3f1ccb5 to
eb189e4
Compare
|
test this please |
Proposed changes
The default decay behavior for
::mw_evaluatePerParticleWithToperatorbreaks any estimator needing per particle Hamiltonian values reported when tmoves were turned on. See #5245. Here we explicity declare and define::mw_evaluatePerParticleWithToperatorto::mw_evaluatePerParticleinstead of::mw_evaluateWithToperatorforCoulombPotential,CoulombPBCAA, andCoulombPBCABso that these energies are not omitted from the Batched EnergyDensityEstimator port.We remove the unnecessary templating on real type from CoulombPotential and split its header into implementation and header to reduce unnecessary recompilation.
What type(s) of changes does this code introduce?
Delete the items that do not apply
Indirect testing of all new code is added with EnergyDensityEstimator unit tests in a coming PR.
Does this introduce a breaking change?
What systems has this change been tested on?
Checklist
Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.