Neighbour analysis for the entire trajectory #251
Conversation
Pull Request Test Coverage Report for Build 1045104153
💛 - Coveralls |
|
I see that the results are stored in private variables. How are they supposed to be used? |
User friendly functions to get the neighbors for the required part of the trajetory
I've updated the structure of the class! This can now be accessed by |
|
Currently it's mostly equivalent to |
In principle, yes. Such code is trivial but I have to repeat this in pretty much all my notebooks. Options to do this only for certain parts of the trajectory is also pretty useful for me
I'll mostly use this to analyze bond breaking/formation for molecules (reactivity), species sensitive radial distribution functions and so on! |
|
|
||
| """ | ||
|
|
||
| def __init__(self, init_structure=None, positions=None, cells=None, num_neighbors=12, **kwargs): |
There was a problem hiding this comment.
I think it'd be extremely useful for this to take any object that implements HasStructure instead of passing all of these things separately. Calling get_structure every step might be a bit slower, but since you're doing essentially what AtomisticGenericJob.get_structure does anyway, it shouldn't be that much.
There was a problem hiding this comment.
If it turns out much slower, then I'd say we add a method roughly like this
@classmethod
def from_structures(cls, has_structure):
structs = list(has_structure.iter_structures())
return cls(structs[0], [s.positions for s in structs], [s.cell for s in structs])to NeighborTraj to achieve the same generality, but offer a small escape hatch for cases where we know get_structure is too slow.
There was a problem hiding this comment.
If it turns out much slower, then I'd say we add a method roughly like this
@classmethod def from_structures(cls, has_structure): structs = list(has_structure.iter_structures()) return cls(structs[0], [s.positions for s in structs], [s.cell for s in structs])to
NeighborTrajto achieve the same generality, but offer a small escape hatch for cases where we knowget_structureis too slow.
I think this is a great way to generalize this for any derivative of HasStructure. However, I don't think building the positions and cells from the individual structures is not that efficient.
So how about something like this:
def __init__(self, has_structure=None, init_struct=None, positions=None, cells=None):
if has_structure is None:
if positions is None or init_struct is None:
raise ValueError()
and then
def _get_neighbors_hs(has_struct, num_neighbors=20, **kwargs):
[struct.get_neighbors() for struct in has_struct.iter_structures()]
which gets called instead of _get_neighbors
There was a problem hiding this comment.
I think we should simply give it a try to use get_structure() everywhere and see if it really is slower. Checking Trajectory._get_structure and your code in _get_neighbors() looks essentially the same to me, so I don't expect a big hit.
If you don't want to do it, I can give it a go tonight.
There was a problem hiding this comment.
Also __init__ needs to accept an init argument as the first argument and pass it to super().__init__(), otherwise recursive loading from HDF will not work.
There was a problem hiding this comment.
I think we should simply give it a try to use
get_structure()everywhere and see if it really is slower. CheckingTrajectory._get_structureand your code in_get_neighbors()looks essentially the same to me, so I don't expect a big hit.If you don't want to do it, I can give it a go tonight.
Sure I'll merge your PR into this one and see what happens!
There was a problem hiding this comment.
Also
__init__needs to accept aninitargument as the first argument and pass it tosuper().__init__(), otherwise recursive loading from HDF will not work.
Not sure what you mean by this. Could you clarify?
There was a problem hiding this comment.
Also
__init__needs to accept aninitargument as the first argument and pass it tosuper().__init__(), otherwise recursive loading from HDF will not work.Not sure what you mean by this. Could you clarify?
When you derive from DataContainer the new __init__ method must be compatible to the original one. One of the ways DataContainer is instantiated is by passing a collection to initialize it, like so
d = DataContainer({'a': 1, 'b': 3, 2: 42})
assert d[1] == 3Because this is used by the DataContainer internally when loading from hdf, subclasses also need to offer this. There's a little bit more explanation here in the attention box.
# Conflicts: # pyiron_atomistics/atomistics/job/atomistic.py
Co-authored-by: Marvin Poul <[email protected]>
| def __init__(self, init_structure=None, positions=None, cells=None, num_neighbors=12, **kwargs): | ||
| """ | ||
|
|
||
| Args: | ||
| init_structure (pyiron_atomistics.atomistics.structure.atoms.Atoms): Any given structure of the trajectory | ||
| positions (numpy.ndarray): The cartesian positions of the trajectories | ||
| cells (numpy.ndarray/None): The varying cell shapes | ||
| num_neighbors (int): The cutoff for the number of neighbors | ||
| **kwargs (dict): Additional arguments to be passed to the `get_neighbors()` routine | ||
| (eg. cutoff_radius, norm_order , etc.) | ||
| """ | ||
| self._init_structure = init_structure | ||
| self._neighbor_indices = None | ||
| self._neighbor_distances = None | ||
| self._neighbor_vectors = None | ||
| self._positions = positions | ||
| self._cells = cells | ||
| self._num_neighbors = num_neighbors | ||
| self._get_neighbors_kwargs = kwargs |
There was a problem hiding this comment.
For __init__ to be compatible a change like this would be necessary. When init is given the other parameters are not set, because presumably they are set in the init read from HDF5. If you want to be complete, you can check that init really contains all the attributes that __init__ normally sets and set those manually that are not in `init.
| def __init__(self, init_structure=None, positions=None, cells=None, num_neighbors=12, **kwargs): | |
| """ | |
| Args: | |
| init_structure (pyiron_atomistics.atomistics.structure.atoms.Atoms): Any given structure of the trajectory | |
| positions (numpy.ndarray): The cartesian positions of the trajectories | |
| cells (numpy.ndarray/None): The varying cell shapes | |
| num_neighbors (int): The cutoff for the number of neighbors | |
| **kwargs (dict): Additional arguments to be passed to the `get_neighbors()` routine | |
| (eg. cutoff_radius, norm_order , etc.) | |
| """ | |
| self._init_structure = init_structure | |
| self._neighbor_indices = None | |
| self._neighbor_distances = None | |
| self._neighbor_vectors = None | |
| self._positions = positions | |
| self._cells = cells | |
| self._num_neighbors = num_neighbors | |
| self._get_neighbors_kwargs = kwargs | |
| def __init__(self, init=None, init_structure=None, positions=None, cells=None, num_neighbors=12, **kwargs): | |
| """ | |
| Args: | |
| init_structure (pyiron_atomistics.atomistics.structure.atoms.Atoms): Any given structure of the trajectory | |
| positions (numpy.ndarray): The cartesian positions of the trajectories | |
| cells (numpy.ndarray/None): The varying cell shapes | |
| num_neighbors (int): The cutoff for the number of neighbors | |
| **kwargs (dict): Additional arguments to be passed to the `get_neighbors()` routine | |
| (eg. cutoff_radius, norm_order , etc.) | |
| """ | |
| super().__init__(init=init) | |
| if init is None: | |
| self._init_structure = init_structure | |
| self._neighbor_indices = None | |
| self._neighbor_distances = None | |
| self._neighbor_vectors = None | |
| self._positions = positions | |
| self._cells = cells | |
| self._num_neighbors = num_neighbors | |
| self._get_neighbors_kwargs = kwargs |
# Conflicts: # pyiron_atomistics/atomistics/structure/neighbors.py
# Conflicts: # pyiron_atomistics/atomistics/structure/neighbors.py
|
Since a |
|
@samwaseda & @sudarsan-surendralal I just saw that the attribute names between |
Co-authored-by: Marvin Poul <[email protected]>
Co-authored-by: Marvin Poul <[email protected]>
…mistics into neighbors_traj
# Conflicts: # pyiron_atomistics/atomistics/structure/neighbors.py
The idea is to easily perform a neighbor analysis over the trajectory of an atomistic simulation. This is something I've been doing in my notebooks for some time which could be useful to others as well. There are also ways to make this more efficient. So please feel free to suggest improvements!