-
Notifications
You must be signed in to change notification settings - Fork 189
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
reaction_methods: new optional keyword exclusion_radius_per_type #4469
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
May I suggest to make the exclusion radius a required keyword, forcing the user to explicitly consider what parameter makes sense. Ideal reactions are rarely executed in Espresso which is why I would not use a default value which will only work for ideal systems, when not only MC moves are performed (but also MD moves). |
@jonaslandsgesell the exclusion radius is still a required keyword. An ideal system system is only assumed if the user provides If the user does not provide an exclusion radius for a particle type, such particles are assumed to have an exclusion radius equal to 0, which I agree that could be potentially dangerous. Since in principle the user can create new particle types after the reaction methods have been initialized, the only alternative way that I can think of is to check it during the particle insertion and throw an error if a particle type without an exclusion radius is found. In practice, this would force the users to define an exclusion radius for all particle types that exist within their simulations. However, such change would force us to make the exclusion radius a required keyword for the widom insertion too, which would be annoying since it is not actually necessary. |
@jonaslandsgesell I have figured out a way of checking that all the existing particle types have a defined excluded radius without having to force the user to define an exclusion radius when using the widom insertion method. I belive that the issue is now solved. |
@pm-blanco thanks for having a look. Short disclaimer: I did not have a deeper look at your changes due to time :) maybe you already have a great solution but here are some things which popped up in my mind when thinking about the exclusion radii.
|
I can add a check during the initialization of the reaction methods to prevent the user to provide an empty dict. It feels a little bit redundant to me since the code already checks that the user has defined an exclusion_radius for all existing particle types during the particle insertion, effectively dealing with this issue. It could however be a nice way to make the user aware in a early stage that he/she should define one exclusion radius per particle type.
In my opinion, using that pair definition of the exclusion radius can complicate its setting up unnecessarily. Since there are only a few established combination rules, I believe that it would be rather more convenient to define an optional keyword e.g. |
743b1f6
to
547098e
Compare
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 for this work! Please find a few suggestions below.
547098e
to
4d2d8b1
Compare
Fixes #4437
Description of changes:
exclusion_radius
has been renamed toexclusion_range
.exclusion_radius
is provided, a keyerror warns the user thatexclusion_radius
is obsolete and the current equivalent keyword isexclusion_range
exclusion_radius_per_type
as an optional argument.exclusion_radius_per_type
, following the Lorentz-Berthelot combining rule.exclusion_range
is used by default.exclusion_radius_per_type
is equal to 0, then the exclusion range of that particle type with any other particle is 0.doc/sphinx/advanced_methods.rst
to explain how to couple the reaction methods to molecular dynamics, including the new features of this PR.src/core/reaction_methods/tests/ReactionAlgorithm_test.cpp
to check the behavior ofexclusion_radius_per_type
ReactionAlgorithm::check_exclusion_range
now breaks if one particle is found within the exclusion range of the inserted particle.I checked the computational efficiency using the benchmark in
maintainer/benchmarks/mc_acid_base_reservoir.py
and this PR improves the MC computational by a 15% compared to the current python branch (MC timing: 1.268e-04 vs 1.395e-04, this PR vs current python, respectively). Probably the gain could be even higher for dense systems.