A new "ndirect" mode for preprocessing #4
Conversation
There was a problem hiding this comment.
Thank you for your contribution! The code looks great. I have several very minor issues, mainly about formatting. The code should be ready to merge once you fix those. In addition, did you test if the new algorithm gives the exact same results as the existing two for the given trajectories?
| @@ -96,7 +96,7 @@ Then, you can use the `preprocess.py` to preprocess the `traj.npz`. It will crea | |||
| python preprocess.py traj.npz graph.npz | |||
| ``` | |||
|
|
|||
| Note that the graph construction is slow especially for large MD trajectories. There two different graph construction algorithms implemented. The default `--backend kdtree` has a linear scaling but only works for orthogonal simulation box. For non-orthogonal simulation, use flag `--backend direct` which has a quadratic scaling. You can also take advantage of the multiprocessing with flag `--n-workers`. For other flags, checkout the help information with `python preprocess.py -h`. | |||
| Note that the graph construction is slow especially for large MD trajectories. There two different graph construction algorithms implemented. The default `--backend kdtree` has a linear scaling but only works for orthogonal simulation box. For non-orthogonal simulation, use flag `--backend direct` or `--backend ndirect` which has a quadratic scaling (for the two choices, the latter is specially efficient for large cells, while the former could be quick for small ones). You can also take advantage of the multiprocessing with flag `--n-workers`. For other flags, checkout the help information with `python preprocess.py -h`. | |||
There was a problem hiding this comment.
Small typos:
Two different graph convolution algorithm -> three
| 'lattices but has quadratic scaling. ' | ||
| 'lattices but has quadratic scaling. "ndirect" is ' | ||
| 'an enhanced method for "direct" which could ' | ||
| 'accelarate the process ofdealing with large lattices.' |
| prep_parser.add_argument('--backend', choices=['kdtree', 'direct'], | ||
| default='kdtree', help='either "kdtree" or "direct", ' | ||
| prep_parser.add_argument('--backend', choices=['kdtree', 'direct', 'ndirect'], | ||
| default='kdtree', help='"kdtree", "direct" or "ndirect" available, ' |
There was a problem hiding this comment.
Remove “available” after “ndirect”
| @@ -169,6 +169,33 @@ def construct_graph(self, traj_coords, lattices, atom_types, target_index): | |||
| 'target_index': target_index, | |||
| 'nbr_lists': nbr_lists, | |||
| 'nbr_dists': nbr_dists} | |||
| elif self.backend == 'ndirect': | |||
| stcs = [Structure(lattice=lattices[i], | |||
There was a problem hiding this comment.
Don’t use such complex list comprehensions. Use a for loop for code readability.
| a, b, c = [np.ceil(2*self.radius/d).astype('int') | ||
| for d in stcs[0].lattice.abc] | ||
| if [a, b, c] != [1, 1, 1]: | ||
| _ = [stc.make_supercell( |
There was a problem hiding this comment.
Use a for loop here. As well as several places below.
| :, 1:1+self.n_nbrs] for stc in tqdm( | ||
| stcs, desc='Generating neighbor index...', disable=not self.verbose)], dtype='int32') | ||
| nbr_dists = np.array([np.sort(stc.distance_matrix)[ | ||
| :, 1:1+self.n_nbrs] for stc in tqdm( |
There was a problem hiding this comment.
Can you reformat your code according to PEP8? There should be whitespaces between 1+ for example. You can do it with automated tools.
Thanks a lot for pointing out and I will fix soon. |
For some large, non-orthogonal box, it could be rather slow using
directmode to preprocess. So in this pr, a new modendirect, using numpyndarrayto produce neighbor list, is developed and has been tested to give the same result as the originaldirectmode.