-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding animate
method to Trajectory
#853
Conversation
prince-mathews
commented
Nov 2, 2022
- Within the ´animate´ method inside the Trajectory class, it wasn't possible to include ´stride´, ´atom_indices´, ´snapshot_indices´ or ´repeat´ arguments.
- Within the ´animate´ method inside the Trajectory class, it wasn't possible to include ´stride´, ´atom_indices´, ´snapshot_indices´ or ´repeat´ arguments.
Pull Request Test Coverage Report for Build 3401805185
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
You had also code for the creation of a class Trajectory:
...
@classmethod
def from_structure_list(cls, structures):
# somehow get all positions, etc. from all structures
return cls(positions, ...) |
Isn't this something the Trajectory class does by itself? |
Mh, I was thinking really turning a list of |
This also deprecates AtomisticGenericJob.animate_structure, because it inherits animate_structure*s* from HasStructure now. The older method has a few more arguments, but it is implemented purely in terms of calling traj = job.trajectory(...) traj.animate_structures(...) Therefore users can replace calls of form job.animate_structure(**traj_args, **anim_args) very easily by job.trajectory(**traj_args).animate_structures(**anim_args)
I've added some code to move |
self.structure is only available on jobs, so we do the check only in the old animate method