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

Refactor bonded interaction parameter handling #4161

Merged
merged 7 commits into from
Mar 25, 2021

Conversation

biermanncarl
Copy link
Contributor

@biermanncarl biermanncarl commented Mar 16, 2021

Fixes #3019, partial fix for #3680, closes #4066

Description of changes:

  • Bonded parameters are now stored inside a boost::variant
  • Most bond types are now in self-contained header files (dependency reversals compared to former state)
  • Most bond kernels have become members of the respective structs
  • Each bond type is represented by its own C++ type (the three tabulated types are now separate, too), which is important because C++ type is now equivalent to bond type
  • Removed the former bond type identification scheme from the C++ core
  • Separate bonded IA cutoff from non-bonded IA cutoff
  • Skip bond loop independently from non-bonded loop

@biermanncarl biermanncarl changed the title Refactor bonded interaction parameter handling WIP - Refactor bonded interaction parameter handling Mar 16, 2021
@@ -90,6 +90,11 @@
#define TINY_OIF_ELASTICITY_COEFFICIENT 1e-10
#endif

/** Maximal number of soft particles per IBM volcons bond (?) */
#ifndef IBM_MAX_NUM
#define IBM_MAX_NUM 1000
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

@biermanncarl biermanncarl Mar 17, 2021

Choose a reason for hiding this comment

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

It's basically a relocation of a constant from here

Copy link
Member

Choose a reason for hiding this comment

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

This constant had to be moved out to help with dependency inversion, IIRC.

src/core/bonded_interactions/angle_cosine.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/angle_cossquare.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/angle_cossquare.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_coulomb.hpp Outdated Show resolved Hide resolved
return std::max(max_cut, cutoff(bond.type, bond.p));
});
auto const max_cut_bonded = boost::accumulate(
bonded_ia_params, -1.,
Copy link
Member

Choose a reason for hiding this comment

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

why do you initialize with -1 and not 0?

Copy link
Member

Choose a reason for hiding this comment

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

I think -1. is the magic value that will be replaced by the more expressive macro BONDED_INACTIVE_CUTOFF in #4066.

Copy link
Member

Choose a reason for hiding this comment

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

Still don't understand this... The minimum value for the maximum cut off is zero and not some magic number.

@@ -99,31 +60,28 @@ double maximal_cutoff_bonded() {
return (any_dihedrals) ? 2 * max_cut_bonded : max_cut_bonded;
}

int bonded_ia_params_num_partners(int bond_id) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
int bonded_ia_params_num_partners(int bond_id) {
int no_of_partners(int bond_id) {

Copy link
Member

Choose a reason for hiding this comment

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

The bonded_ia_params_ prefix is used in this function and the ones above and below because it's acting on the variant called bonded_ia_params. This will become clear once we add the correct doxygen group delimiters. We could even move these functions to src/script_interface.

Copy link
Member

Choose a reason for hiding this comment

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

Function moved to src/script_interface/interactions/bonded.hpp and modified to call a new function with signature number_of_partners(Bonded_ia_parameters const &iaparams) in src/core/bonded_interactions/bonded_interaction_data.cpp.

@biermanncarl biermanncarl force-pushed the fix-3019 branch 2 times, most recently from 1e4f86f to bcefbc8 Compare March 19, 2021 15:12
- store bonded parameters in a boost::variant
- make force/energy kernels methods of the bond structs
- use boost::serialization for MPI communication

Co-authored-by: Jean-Noël Grad <[email protected]>
@jngrad jngrad changed the title WIP - Refactor bonded interaction parameter handling Refactor bonded interaction parameter handling Mar 23, 2021
@jngrad jngrad added this to the Espresso 4.2 milestone Mar 23, 2021
jngrad
jngrad previously approved these changes Mar 23, 2021
Copy link
Member

@jngrad jngrad left a comment

Choose a reason for hiding this comment

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

good to go from my side

Copy link
Member

@KaiSzuttor KaiSzuttor left a comment

Choose a reason for hiding this comment

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

Please check that all types follow our naming convention (SnakeCase) and adjust the naming of the bond classes (remove the Parameters suffix)

src/core/bonded_interactions/angle_cosine.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/angle_cosine.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/angle_harmonic.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_coulomb_sr.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_coulomb_sr.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_interaction_data.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_tab.cpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_tab.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_tab.hpp Outdated Show resolved Hide resolved
src/core/bonded_interactions/bonded_tab.hpp Outdated Show resolved Hide resolved
@KaiSzuttor KaiSzuttor added the automerge Merge with kodiak label Mar 25, 2021
@kodiakhq kodiakhq bot merged commit 3097773 into espressomd:python Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bond_parameters should be a variant
4 participants