MDAnalysis / membrane-curvature

MDAnalysis tool to calculate membrane curvature.
https://membrane-curvature.readthedocs.io/
GNU General Public License v3.0
29 stars 6 forks source link

Investigate effect of `np.float32` cast in code performance. #42

Closed ojeda-e closed 3 years ago

ojeda-e commented 3 years ago

From PR #40 and comments.
Function derive_surface includes factor = np.float32(n_cells / max_width) in line 30, which is not clear. Investigate performance of factor = np.float32(n_cells / max_width) vs, factor = n_cells / max_width.

ojeda-e commented 3 years ago

This issue was opened during refactoring. The previous code used operations with grid_z_coordinates = np.zeros([n_cells, n_cells]) and grid_norm_unit = np.zeros([n_cells, n_cells]) that initially offered faster calculation by adding factor = np.float32(n_cells / max_width). Since operations between these arrays are used anymore (it was used in a for loop to iterate over frames here, in line 57 ), IMO this issue makes no sense anymore. Hence, considering closing it.

Any objections @IAlibay @orbeckst @lilyminium?

orbeckst commented 3 years ago

Sounds sensible.

Am 7/18/21 um 21:18 schrieb Estefania Barreto-Ojeda @.***>:

 This issue was opened during refactoring. The previous code used operations with grid_z_coordinates = np.zeros([n_cells, n_cells]) and grid_norm_unit = np.zeros([n_cells, n_cells]) that initially offered faster calculation by adding factor = np.float32(n_cells / max_width). Since operations between these arrays are used anymore (it was used in a for loop to iterate over frames here, in line 57 ), IMO this issue makes no sense anymore. Hence, considering closing it.

Any objections @IAlibay @orbeckst @lilyminium?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

IAlibay commented 3 years ago

+1 sounds reasonable.

ojeda-e commented 3 years ago

Thanks @orbeckst @IAlibay , officially closing this issue.