Skip to content
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

Merged
merged 7 commits into from
Nov 5, 2022
Merged

Adding animate method to Trajectory #853

merged 7 commits into from
Nov 5, 2022

Conversation

prince-mathews
Copy link
Contributor

  • 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.
@prince-mathews prince-mathews requested a review from pmrv November 2, 2022 10:47
@coveralls
Copy link

coveralls commented Nov 2, 2022

Pull Request Test Coverage Report for Build 3401805185

  • 13 of 31 (41.94%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+9.0e**-05%**) to 68.647%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyiron_atomistics/atomistics/job/atomistic.py 1 4 25.0%
pyiron_atomistics/atomistics/structure/has_structure.py 12 27 44.44%
Files with Coverage Reduction New Missed Lines %
pyiron_atomistics/atomistics/job/atomistic.py 1 75.6%
Totals Coverage Status
Change from base Build 3361607611: 9.0e-05%
Covered Lines: 12086
Relevant Lines: 17606

💛 - Coveralls

Copy link
Contributor

@pmrv pmrv left a 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!

@pmrv
Copy link
Contributor

pmrv commented Nov 3, 2022

You had also code for the creation of a Trajectory from a list of structures, right? Can you add that as well as a method to Trajectory? Something like

class Trajectory:
  ...
  @classmethod
  def from_structure_list(cls, structures):
     # somehow get all positions, etc. from all structures
     return cls(positions, ...)

@prince-mathews
Copy link
Contributor Author

You had also code for the creation of a Trajectory from a list of structures, right? Can you add that as well as a method to Trajectory? Something like

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?
I don't have a code, I just provide the Trajectory class the initial structure, positions of the structures I want to see in the trajectory and the Trajectory is created by the class itself.

@niklassiemer niklassiemer added the format_black reformat the code using the black standard label Nov 4, 2022
@pmrv
Copy link
Contributor

pmrv commented Nov 5, 2022

Mh, I was thinking really turning a list of Atoms objects into a trajectory. But I remember now that the Trajectory class only works with "canonical" structures (equal number of particles everywhere). I'll add some stuff to this branch.

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)
@pmrv
Copy link
Contributor

pmrv commented Nov 5, 2022

I've added some code to move animate over to HasStructure. That means that a lot more jobs can now be animated (StructureContainer/TrainingContainer/StructureStorage/TrainingStorage/etc.).

pmrv added 2 commits November 5, 2022 20:38
self.structure is only available on jobs, so we do the check only in the
old animate method
@pmrv pmrv added enhancement New feature or request integration Start the notebook integration tests for this PR labels Nov 5, 2022
@pmrv pmrv merged commit 836b47f into master Nov 5, 2022
@delete-merged-branch delete-merged-branch bot deleted the trajectory branch November 5, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request format_black reformat the code using the black standard integration Start the notebook integration tests for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants