Refactoring of Ewald summation policies and Energy class #247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rc83
reviewed
Feb 10, 2020
There was a problem hiding this comment.
Nice refactorization. Pairing strategy will fit there nicely in the future. Short summary follows:
- I am a proponent of a factory method, if enumeration is used to select and create a particular polymorphic instance.
- It should be possible to use automatic string-to-enum mapping in JSON.
- Can a group support
g.at(i)andg[i]instead of awkward(g.begin() + i)syntax? - A lot of work should be done to comply with our naming standard. I picked only that cases that annoyed me most. ;-)
| double kappa2 = 0; //!< Squared inverse Debye screening length | ||
| double alpha = 0; | ||
| double const_inf = 0; | ||
| double check_k2_zero = 0; |
There was a problem hiding this comment.
Use a more descriptive name. It is difficult to guess what this variable of type double actually contains.
There was a problem hiding this comment.
will talk more to @bjornstenqvist
| private: | ||
| EwaldData data; | ||
| Policy policy; | ||
| std::shared_ptr<EwaldPolicyBase> policy; //!< Policy for updating k-space |
There was a problem hiding this comment.
Just a matter of taste. I am used to call it “strategy” when using composition in pure object-oriented programming style, and “policy” if generic programming (i.e., templates) is involved.
There was a problem hiding this comment.
There are many options, "method", "scheme", "policy", "strategy", "plan". Let's talk about this.
rc83
approved these changes
Feb 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This refactors Ewald policies and
Energy::Ewaldwith separate policy objects for PBC, IPBC with and without Eigen optimisations. The policy is changed from static to dynamic and unittests and an ewald example is provided. Effectively this means that all Ewald code has moved from headers to cpp.spherical_sum