Evaporation dependent perturbed Lennard Jones potential#127
Conversation
|
FYI: every time you push a commit, the actions will take a very long time to run so I recommend only doing that when you have a complete set of changes that you want tested and/or reviewed. For example, this commit will not actually test any of your code because you've only added header files. |
There was a problem hiding this comment.
This can go at the bottom of the PerturbedLennardJonesEvap.cc file, it doesn't need to be separate.
There was a problem hiding this comment.
Same here for the GPU version.
| self._attraction_scale_factor_data = numpy.asarray( | ||
| attraction_scale_factor_data, dtype=numpy.float64 | ||
| ) | ||
| self._attraction_scale_factor_shape = numpy.asarray( | ||
| attraction_scale_factor_shape, dtype=numpy.uint32 | ||
| ) |
There was a problem hiding this comment.
I'm not positive since the arguments of this aren't documented, but can't you get the shape of the scale factor data from the array, rather than storing a separate array?
| lj1(params.epsilon_x_4 * params.sigma_6 * params.sigma_6), | ||
| lj2(params.epsilon_x_4 * params.sigma_6), rcutsq(rcut * rcut), rwcasq(params.rwcasq), | ||
| sigma_6(params.sigma_6), m_energy_shift(energy_shift), m_variant(variant) |
There was a problem hiding this comment.
The way this class is being constructed is quite confusing because of the mix of the parameter struct. Is it meant to operate on all particles in the system with the same sigma and epsilon? If yes, you would be better off just passing epsilon and sigma to the class, and packing them up into a struct later if it's really needed (it probably isn't).
Also, things like lj1, lj2, rcutsq, rwcasq, and sigma_6 are temporary variables that should be computed before they are needed not as class members.
| auto clamp_scaled_y = [](Scalar y, Scalar height, Scalar ylo) | ||
| { | ||
| const Scalar s = (y - ylo) / (height - ylo); | ||
| if (!(s > Scalar(0.0))) | ||
| return Scalar(0.0); | ||
| if (s > Scalar(1.0)) | ||
| return Scalar(1.0); | ||
| return s; | ||
| }; |
There was a problem hiding this comment.
Does this code really need to be a lambda function? It's only used once.
| namespace azplugins | ||
| { | ||
|
|
||
| struct PairParametersPerturbedLennardJonesEvap |
There was a problem hiding this comment.
See comment above about constructor. I think this parameter type is probably not needed unless you want to be able to set different epsilon and sigma for different pairs of particles.
| Scalar scaleTime(uint64_t timestep) const | ||
| { | ||
| return Scalar(static_cast<Scalar>(timestep) / m_time_scale_factor); | ||
| } |
There was a problem hiding this comment.
Where is this used and does it really need to be part of the public API of the class? It looks like it could be one line of code somewhere.
| Scalar getEpsilon() const | ||
| { | ||
| return epsilon_x_4 / Scalar(4.0); | ||
| } | ||
|
|
||
| Scalar getSigma() const | ||
| { | ||
| return std::pow(sigma_6, Scalar(1.0) / Scalar(6.0)); | ||
| } |
There was a problem hiding this comment.
These can become simple getters / setters on epsilon and sigma if there is only one value.
There was a problem hiding this comment.
Similar comments as for CPU code apply to this file.
| from hoomd.azplugins import _azplugins | ||
|
|
||
|
|
||
| class VariantInterpolated(_azplugins.VariantInterpolated, hoomd.variant.Variant): |
There was a problem hiding this comment.
I'm confused by the double inheritance pattern here, and I'm not sure if what it is doing is right. Did you get this from somewhere else in HOOMD?
There was a problem hiding this comment.
Yes, scalar.py in the variant directory of HOOMD uses the same pattern.
2 participants
No description provided.