-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Langevin thermostat #296
base: master
Are you sure you want to change the base?
Langevin thermostat #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the effort and work implementing this new functionality! Looks good overall to me! I left some comments and feedback for discussions.
binData.mol_counts.insert(count); | ||
binData.velocities.insert(v_avg); | ||
|
||
//compute CAM average first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is CAM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an averaging method, see 10.1002/fld.2487 and 10.1016/j.jcp.2003.10.021
binData.v_cam.insert(v_cam); | ||
} | ||
|
||
//compute SAM over CAM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is SAM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an averaging method, see 10.1002/fld.2487 and 10.1016/j.jcp.2003.10.021
* Ring buffer that stores up to n entries. Will override oldest entry once n+1 elements are inserted. | ||
* */ | ||
template<typename T> | ||
struct NBuffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not check 100% the usage of this but maybe have a look at utils/Accumulator.h to see if there is already functionality available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using it but failed to compile afterwards due to redefinition in TemperatureControl.
Moving include directives to cpp files and using predeclarations did not help.
So, for now I'll keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me.
It would be nice to add an unit test (if possible) and an example xml config.
Just a question for my understanding:
The integrator has an active region option. What if not all particles are within an active region. How are they handled?
As a note:
The whole temperature control stuff (which is closely related to the NVT/NVE ensemble issue) is a bit messy in ls1. See e.g. here were the _temperatureControl
is on a different else if
level than _thermostatType
.
This was part of your thesis, right? If possible, you could also add a permanent link to your thesis to the description of this PR, if your thesis is available e.g. via your university's library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor points on just making sure we have material to read about the Langevin integrator.
The key point is that you use a lot of variable names which make no sense to somebody unfamiliar with what you are doing. If it is clear from the reading material what these variables are, then that's okay. Otherwise, make variable names more verbose.
Btw, I did not check this too thoroughly so go to somebody else for PR approval :(
* New time integrator that deviates from the standard Verlet integration scheme. | ||
* Equations of motion are changed to follow Langevin equations. | ||
* This is a stochastic integration scheme, which is incorporated as random "kicks" from the connected heat bath. | ||
* | ||
* This is to be used with the TemperatureObserver, which disables the normal velocity scaling and defines | ||
* the regions in which the Langevin Thermostat should be active. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have a reference of a paper/some material to make this easier to follow.
ParticleContainer *particleContainer, BinData &binData); | ||
|
||
/** | ||
* Computes the SAM over the CAM average of the mean velocity for the specified region. (SCM method) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is SAM and CAM mentioned in the papers? If not provide full name. Otherwise, mark this comment as resolved.
Description
Added a Langevin Thermostat which follows the Langevin equations of motion.
This required adding a new integrator as well as a dummy thermostat to disable the default velocity scaling.
The Integrator handles the actual thermostating of the system.
The dummy thermostat can measure the temperature in selected regions.
added xml parsing and init for new integrator in simulation class
added xml parsing and init for thermostat in simulation class
added public getter for thermostat in simulation class
Resolved Issues
How Has This Been Tested?