Use current structure instead of output to update previous structure #483
Conversation
Pull Request Test Coverage Report for Build 1602851941
💛 - Coveralls |
There was a problem hiding this comment.
Lgtm. That timing plot in the pr description is a beautiful thing. @raynol-dsouza i guess this is very nice news for every Protocol that exploits lammps
It's the output values ( |
|
I am not at all into this so this might be completely off. However, do I understand correctly that this PR just uses the last structure which had been set to the job as the last structure? Is it not also possible to generate multiple structures per run call? Then we would ignore them wouldn't we? |
The structure comparison is done in order to see which values have changed between two runs. So as long as it allows to see which values have to be updated before the next run, it doesn't matter how many structures there are. |
There was a problem hiding this comment.
The interactive mode is divided in two parts, a beginners mode job.structure = new_structure and an experts mode job.interactive_positions_setter(new_positions). The expert mode is designed for efficiency and the beginners mode is designed for reliability. With the current changes to the beginners mode the structure of the job object job.structure is no longer the same as the structure in the interactive job - for example when running lammps with calc_minimize() in interactive mode. I think this is a rather risky modification.
Yeah ok I remember the discussions. But that means in |
|
@jan-janssen I added the changes we discussed yesterday. |
| @@ -156,7 +156,7 @@ def interactive_index_organizer(self): | |||
| self.interactive_indices_setter(self._structure_current.indices) | |||
|
|
|||
| def interactive_cell_organizer(self): | |||
| if not np.allclose( | |||
| if self._generic_input["calc_mode"] != 'static' or not np.allclose( | |||
There was a problem hiding this comment.
This is not necessary for calc_minimize() only calc_minimize(pressure=0) right? And analogously it is not necessary for nvt and nve but npt.
There was a problem hiding this comment.
Is the information about nvt, nve etc. included in the input?
There was a problem hiding this comment.
Not yet but we could include it in https://github.com/pyiron/pyiron_atomistics/blob/master/pyiron_atomistics/atomistics/job/atomistic.py#L226
There was a problem hiding this comment.
So we could go ahead with this version for now, right? I don't think communicating the cell size would significantly change something.
The issue is discussed in #481. Now I also did a performance test and got the following results:
This means in particular that after 1000 steps (since each iteration contains 10 steps) the new implementation is going to be around 30 times faster than the old one.
closes #481