search

royallgroup / TCC

The Topological Cluster Classification algorithm
https://royallgroup.github.io/TCC/
GNU General Public License v3.0
12 stars 5 forks source link

Add cell list to simple bond calculation #29

Open merrygoat opened 6 years ago

merrygoat commented 6 years ago

To speed up determination of simple (non-voronoi) bond lengths. Add a cell list.

merrygoat commented 6 years ago

This would be useful for Josh. This should be easy to do now the cell list has been sorted.

merrygoat commented 6 years ago

The cell list has been implemented in 86fc78140b9c65f0476ab1d69dac06a26c0e8cbd and it produced the same bond network as the simple cell list. However, the simple bonds method fills the bNums array in ascending order of bond number while the cell list fills it in a random order. This seems to have an effect on some cluster identification, in testing 12D, 12E and 13A, though it may be more.

It is not clear whether it is these cluster methods directly or one of the components they access but whatever the reason it is very bad that the code is non deterministic based on the order of the bonds since it is detecting different numbers of clusters from the same bond network.

As a quick fix, tt would be OK to sort the bNums array apart from the fact that the this wouldn't also sort the bond_lengths array which stores the corresponding bond lengths. Since the arrays can't be sorted simultaneously (or at least not easily) the bond lengths would then not correspond to the correct bonds.

merrygoat commented 6 years ago

This turns out to be more complex than initially expected. The reason for the bug is that the 12D, 12E and 13A methods take an argument from their parent cluster to improve performance. This is somehow not safe to cluster order though. The proper fix to this is to refactor the methods and split them from their parent methods since there is no reason each cluster type should not be detected individually. 12E is finally fixed in 6668fc693a7c1ba0396c308d503cbdacf399626a. 13A and 12D are next to be addressed.

tranqui commented 6 years ago

Was this resolved @merrygoat? The comments above indicate it was fixed, but then it was reopened?

merrygoat commented 6 years ago

I closed it by accident I think, It is not yet resolved. This requires refactoring of some of the more complex clusters which is quite a big job.