Adding Chebyshev anisotropic pair potential by mzf0069 · Pull Request #121 · mphowardlab/azplugins

mzf0069 · 2026-01-26T22:36:43Z

No description provided.

mphoward

This is a good start! Here are some comments, let me know what questions you have.

src/ChebyshevTensorAnisotropicPairPotential.h

mphoward

This is going in a good direction. Let's finalize the interface, especially the constructor, then start to implement that.

src/ChebyshevTensorAnisotropicPairPotential.h

mphoward · 2026-02-10T02:55:31Z

src/ChebyshevTensorAnisotropicPairPotential.h

+                                            const std::vector<Scalar>& r0_data,
+                                            const std::vector<unsigned int>& terms,
+                                            const std::vector<Scalar>& coeffs);
+


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.

mphoward · 2026-02-10T02:57:25Z

src/ChebyshevTensorAnisotropicPairPotential.h

+        }
+
+    /// r0 data (theta, phi, alpha, beta, gamma) (N x 6)
+    const GPUArray<Scalar>& getR0Data() const


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.

mphoward · 2026-02-10T02:57:57Z

src/ChebyshevTensorAnisotropicPairPotential.h

+    /// Allocate storage for term list and coefficients.
+    void resizeTerms(unsigned int Nterms);


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.

mphoward · 2026-02-10T02:58:19Z

src/ChebyshevTensorAnisotropicPairPotential.h

+    protected:
+    void computeForces(uint64_t timestep) override;
+
+    private:


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.

mphoward

Looks good to me, let's start on the implementation!

mphoward · 2026-02-17T15:12:59Z

src/ChebyshevAnisotropicPairPotential.h

+                                      unsigned int Nterms,
+                                      const unsigned int* terms,
+                                      const Scalar* coeffs);


I like to follow the convention that the number of items comes after the pointers

Suggested change

unsigned int Nterms,

const unsigned int* terms,

const Scalar* coeffs);

const unsigned int* terms,

const Scalar* coeffs,

unsigned int Nterms);

mphoward · 2026-02-17T15:14:32Z

src/ChebyshevAnisotropicPairPotential.h

+                                      const Scalar* r0_data,
+                                      const unsigned int* r0_shape,


Logically, should these come before the things related to the potential approximation or after? It doesn't really matter, beyond which way it makes more sense for you to read them.

mphoward · 2026-02-17T15:15:43Z

src/ChebyshevAnisotropicPairPotential.h

+    // neighbor list object
+    std::shared_ptr<hoomd::md::NeighborList> m_nlist;


Member variables should be documented like this in doxygen style

Suggested change

// neighbor list object

std::shared_ptr<hoomd::md::NeighborList> m_nlist;

std::shared_ptr<hoomd::md::NeighborList> m_nlist; //!< Neighbor list

mphoward · 2026-02-17T15:17:54Z

src/ChebyshevAnisotropicPairPotential.h

+    // intenal r0 linear interpolator
+    std::unique_ptr<LinearInterpolator5D> m_r0_interp;
+
+    // r0_data
+    GPUArray<Scalar> m_r0_data;
+
+    std::array<unsigned int, 5> m_r0_shape;


We should discuss how the linear interpolator will work. It may not be necessary to store a separate variable for it, and instead just construct it each time it is needed because it will be a lightweight wrapper around the data and the shape.

Noting: if the shape is stored in an array, it will need to be copied into the interpolator to pass it to the GPU.

mphoward · 2026-02-17T15:18:17Z

src/ChebyshevAnisotropicPairPotential.h

+    GPUArray<Scalar> m_coeffs;
+
+    // number of terms
+    unsigned int m_Nterms = 0;


This doesn't need a default value assigned to it

Suggested change

unsigned int m_Nterms = 0;

unsigned int m_Nterms;

mphoward

This is moving in a good direction! Please see my comments and let me know what questions you have.

mphoward · 2026-03-06T16:57:50Z

src/pytest/test_chebyshev.py

+    domain = numpy.zeros((5, 2), dtype=numpy.float64)
+    terms = numpy.zeros((2, 6), dtype=numpy.uint32)


I would suggest giving the domain and terms some real values, not just zeros, as it may cause problems later once we start implementing things.

mphoward · 2026-03-06T16:59:13Z

src/pytest/test_chebyshev.py

+    assert pot._cpp_obj.n_terms == 2
+    assert numpy.isclose(pot._cpp_obj.r_cut, r_cut)


_cpp_obj is an implementation detail, so we should not test it in detail. The check above that it exists and is not None is OK though.

Suggested change

assert pot._cpp_obj.n_terms == 2

assert numpy.isclose(pot._cpp_obj.r_cut, r_cut)

src/ChebyshevAnisotropicPairPotential.cc

src/ChebyshevAnisotropicPairPotential.h

src/module.cc

mphoward · 2026-03-06T17:14:58Z

src/pair.py

+    @property
+    def r_cut(self):
+        """Cut-off distance in approximation domain"""
+        return self._r_cut


You shouldn't need a property for the ones coming from C++, but you need to use the ParameterDict correctly for those.

Suggested change

@property

def r_cut(self):

"""Cut-off distance in approximation domain"""

return self._r_cut

mphoward · 2026-03-06T17:15:15Z

src/pair.py

+    @property
+    def n_terms(self):
+        """Number of terms."""
+        return int(self._terms.shape[0])


Same here, this should come from C++ automatically.

Suggested change

@property

def n_terms(self):

"""Number of terms."""

return int(self._terms.shape[0])

mphoward · 2026-03-06T17:16:06Z

src/pair.py

+    @property
+    def r0_shape(self):
+        """r0 table shape."""
+        return tuple(int(x) for x in self._r0_data.shape)


We will want to discuss how to expose the array quantities later on. However, I note for now that we don't want to have a special shape property for r0. It should be a NumPy inside this class, so it should work like r0.shape.

src/pair.py

Add header for Chebyshev anisotropic pair potential

a631d5a

mzf0069 requested a review from mphoward January 26, 2026 22:37

mphoward reviewed Jan 30, 2026

View reviewed changes

src/ChebyshevTensorAnisotropicPairPotential.h Outdated Show resolved Hide resolved

src/ChebyshevTensorAnisotropicPairPotential.h Outdated Show resolved Hide resolved

src/ChebyshevTensorAnisotropicPairPotential.h Outdated Show resolved Hide resolved

Update header interface

c0323f1

mzf0069 requested a review from mphoward February 5, 2026 23:04

mphoward reviewed Feb 10, 2026

View reviewed changes

Update header interface

189f25f

mzf0069 requested a review from mphoward February 16, 2026 15:20

mphoward reviewed Feb 17, 2026

View reviewed changes

Update Chebyshev anisotropic pair potential and add linear interpolant

67e1bdc

mzf0069 requested a review from mphoward February 27, 2026 16:05

mphoward reviewed Mar 6, 2026

View reviewed changes

Update linear interpolation, and initiate compute force

45a1740

mzf0069 requested a review from mphoward March 19, 2026 16:29

		/// Allocate storage for term list and coefficients.
		void resizeTerms(unsigned int Nterms);

		// neighbor list object
		std::shared_ptr<hoomd::md::NeighborList> m_nlist;

		domain = numpy.zeros((5, 2), dtype=numpy.float64)
		terms = numpy.zeros((2, 6), dtype=numpy.uint32)

		assert pot._cpp_obj.n_terms == 2
		assert numpy.isclose(pot._cpp_obj.r_cut, r_cut)

Conversation

mzf0069 commented Jan 26, 2026

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants