Adding Chebyshev anisotropic pair potential #121
Conversation
mphoward
left a comment
mphoward
left a comment
There was a problem hiding this comment.
This is a good start! Here are some comments, let me know what questions you have.
mphoward
left a comment
mphoward
left a comment
There was a problem hiding this comment.
This is going in a good direction. Let's finalize the interface, especially the constructor, then start to implement that.
|
|
||
| class LinearInterpolator5D; | ||
|
|
||
| class PYBIND11_EXPORT ChebyshevTensorAnisotropicPairPotential : public ForceCompute |
There was a problem hiding this comment.
Based on your comments, please rename this class, the file, and the include guard to ChebyshevAnisotropicPairPotential
| class PYBIND11_EXPORT ChebyshevTensorAnisotropicPairPotential : public ForceCompute | |
| class PYBIND11_EXPORT ChebyshevAnisotropicPairPotential : public ForceCompute |
| const std::vector<Scalar>& r0_data, | ||
| const std::vector<unsigned int>& terms, | ||
| const std::vector<Scalar>& coeffs); | ||
|
|
There was a problem hiding this comment.
r0_data should be a raw pointer to Scalar so that we can copy the data in-place from a NumPy array. You will need a second raw pointer or a std::vector that stores the dimensionality of r0_data along each of the 5 dimensions.
terms and coeffs also need to be raw pointers, and you'll need one more variable that is the number of terms.
Last, domain should probably also be a raw pointer to a regular Scalar so you can accept a NumPy array that is (6,2) in shape. Otherwise, it can be a std::vector<Scalar2>, and you can copy into it.
| } | ||
|
|
||
| /// r0 data (theta, phi, alpha, beta, gamma) (N x 6) | ||
| const GPUArray<Scalar>& getR0Data() const |
There was a problem hiding this comment.
Do you want to be expose the R0Data data directly after you construct the class? Same questions go for some of the members below that return GPUArray. You have to decide whether you are OK with the user modifying their contents or not.
| /// Allocate storage for term list and coefficients. | ||
| void resizeTerms(unsigned int Nterms); |
There was a problem hiding this comment.
Is this something we want the user to be able to do? It seems like you wouldn't want them to do that themselves, most likely. If it's a method the class needs to trigger, though, it can be protected.
| protected: | ||
| void computeForces(uint64_t timestep) override; | ||
|
|
||
| private: |
There was a problem hiding this comment.
Most of the members in this section will need to be protected not private if the GPU version of the class is going to access them.
2 participants
No description provided.