[JOSS] Coordination Numbers and Spheres

[RMeli]

getCoordinationNumberSpheres()

There seem to be some code duplication in getCoordinationNumberSpheres(). The code after scale= is the same for whichever scale is chosen. Additionally, cnsp is initialized and computed, but not used in the final result.

kallisto/src/kallisto/methods.py

Line 102 in 4e03cd5

cnsp = np.zeros(shape=(nat,), dtype=np.float64)

I suggest to move the following code

kallisto/src/kallisto/methods.py

Lines 115 to 133 in 4e03cd5

	for i in range(nat):
	ia = at[i] - 1
	for j in range(nat):
	if i is j:
	continue
	ja = at[j] - 1
	dx = coords[j][0] - coords[i][0]
	dy = coords[j][1] - coords[i][1]
	dz = coords[j][2] - coords[i][2]
	rSquared = dx * dx + dy * dy + dz * dz
	if rSquared > threshold:
	continue
	r = np.sqrt(rSquared)
	rco = scale * (rcov[ia] + rcov[ja])
	eni = pauling_en[ia]
	enj = pauling_en[ja]
	den = k4 * np.exp(-((np.abs(eni - enj) + k5) ** 2) / k6)
	damp = den * 0.5 * (1 + special.erf(-kn * (r - rco) / rco))
	cnsp[i] += damp

and call it with scale=2 and scale=3. With such changes, the code will better reflect the definition of CNSP_i^(3,2) given in the documentation.

getCoordinationNumbers()

There is some code duplication in getCoordinationNumbers

kallisto/src/kallisto/methods.py

Lines 24 to 37 in 4e03cd5

	for i in range(nat):
	ia = at[i] - 1
	for j in range(nat):
	if i is j:
	continue
	dx = coords[j][0] - coords[i][0]
	dy = coords[j][1] - coords[i][1]
	dz = coords[j][2] - coords[i][2]
	rSquared = dx * dx + dy * dy + dz * dz
	if rSquared > threshold:
	continue
	ja = at[j] - 1
	r = np.sqrt(rSquared)
	rco = rcov[ia] + rcov[ja]

However, writing a common function supporting all the different cntypes could possibly obfuscate the implementation so it is up to you if you want to unify them.

Documentation

In the documentation I'd mention how the parameters k0, k1, k2, k3 are obtained. Could you also link the following bit of text (see Applications part) to the corresponding section?

Suggested TODOs

• Remove unused cnsp calculation in getCoordinationNumberSpheres()
• Merge duplicate code in getCoordinationNumberSpheres()
• Mention how the parameters k0, k1, k2, k3 are obtained (in the documentation)
• Link the text (see Applications part) to the corresponding section

---

JOSS Review

[RMeli]

Even better for getCoordinationNumberSpheres(), you could compute cnsp2 and cnsp3 within the same loop.

[f3rmion]

Hi @RMeli,

for a long time, I was also not really happy with the name of the CNSP, as the definition is not really a sphere but rather a shell. So I have now updated the name of this feature to be "Proximity Shell" (Prox) and also added the possibility of user input via the size flag (default: 2->3). In the documentation, I have separated the CN and the Prox part accordingly. In addition, I have added how the parameters of the CN have been fitted and cleaned up some sections in the code.

[RMeli]

Less code and more general, that's great! ;)