Skip to content
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

Add test for global Nosé-Hoover thermostat + refactor #96

Merged
merged 7 commits into from
Mar 29, 2022

Conversation

danielhollas
Copy link
Contributor

No description provided.

@danielhollas danielhollas added refactor No functional changes, just refactoring or cleaning up the code. testing Any changes to Github Actions or testing scripts. labels Mar 27, 2022
@danielhollas danielhollas self-assigned this Mar 27, 2022
@codecov
Copy link

codecov bot commented Mar 27, 2022

Codecov Report

Merging #96 (32e1f3d) into master (b13e6e5) will increase coverage by 0.80%.
The diff coverage is 86.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   84.74%   85.55%   +0.80%     
==========================================
  Files          41       41              
  Lines        5783     5780       -3     
==========================================
+ Hits         4901     4945      +44     
+ Misses        882      835      -47     
Impacted Files Coverage Δ
src/geom_analysis.F90 100.00% <ø> (ø)
src/modules.F90 37.60% <ø> (ø)
src/init.F90 72.66% <55.55%> (+3.77%) ⬆️
src/nosehoover.F90 92.70% <85.92%> (+2.27%) ⬆️
src/analysis.F90 96.84% <100.00%> (-0.25%) ⬇️
src/ekin.F90 100.00% <100.00%> (ø)
src/potentials.F90 99.48% <100.00%> (ø)
src/utils.F90 100.00% <100.00%> (ø)
src/vinit.F90 97.72% <100.00%> (ø)
src/mdstep.F90 100.00% <0.00%> (+1.09%) ⬆️
... and 1 more

@danielhollas danielhollas requested a review from suchanj March 27, 2022 03:31
@danielhollas danielhollas marked this pull request as ready for review March 27, 2022 03:31
call fatal_error(__FILE__, __LINE__, &
'Invalid NHC thermostat parameter')
end if
end subroutine check_nhc_parameters

subroutine nhc_init()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This init function was quite messy so I've broken in down to several pieces in individual functions. I also moved keyword sanity checks related to NHC here from init.F90.

end do
if (tau0 > 0) Qm(1) = ams ! see tuckermann,stat.mech.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I made an actual functional change for PIMD with staging transformation. Previously, if tau0 was not set in the input file, the NHC mass for the staging centroid would be the same as for the other beads. Not sure whether this has real consequence, I'll do some testing later. Need to read the Tuckermann again to better understand where these formulas are coming from.

if (ams < 0 .and. tau0 < 0) then
call set_yoshida_weights(nyosh)

! Not sure if we should have this default value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Štěpán pointed out that the default tau0=0.001 ps was designed primarily for PIMD thermostatting. However, everyone uses it for regular MD simulations which is not ideal ... But this comes to a need for better education of our users. Maybe we should write down some pamphlet explaining global/local NHC or recommend using Langevine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very good point, I've been worried about that for some time, but didn't know how to approach it.
I would suggest for now to remove the default value for classical MD and require it to be set explicitly in the input file, which should at least alert the users for it's importance a bit. What do you think? We could maybe keep the default value for PIMD.

Writing something up would be great, perhaps I could do a seminar in our group here and than share the slides.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of default value might be the way. It's an additional input section so the user should know what he wants. I would personally set default values for both PIMD (as is) and MD (e.g. global thermostat with tau 1.0 ps), however doing it just for PIMD is better - if somebody tries to recalculate old input, it will be backwards compatible in a sense that ABIN won't change the constant on its own.

Any material would be great, also adding the task to my to-do list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@suchanj I've opened #129 and #130 to investigate optimal setting for NHC thermostat in the context of classical MD.

@danielhollas danielhollas merged commit f0ad0b6 into master Mar 29, 2022
@danielhollas danielhollas deleted the nhc-global branch March 29, 2022 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor No functional changes, just refactoring or cleaning up the code. testing Any changes to Github Actions or testing scripts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants