Move structure.calc setting into the initializer #1068
Conversation
Pull Request Test Coverage Report for Build 5292881233
💛 - Coveralls |
|
I think this was probably just a hiccup on the CI side, sp I'm just going to re-run for now. |
| @@ -50,8 +50,6 @@ def run_static(self): | |||
| self.server.run_mode = pre_run_mode | |||
|
|
|||
| def run_if_interactive(self): | |||
| if self.structure.calc is None: | |||
| self.set_calculator() | |||
| super(AseJob, self).run_if_interactive() | |||
There was a problem hiding this comment.
I thought we need this here. For example when running and MD calculation we would update the structure internally so while the first structure might have a calculator the next structure might not.
There was a problem hiding this comment.
I haven't gotten to testing it yet, but what you say sounds reasonable. I'll add a test when I get back to this PR and we'll find out. If so, it should be here and in the new location.
There was a problem hiding this comment.
So you were spot on that this was necessary. The following breaks when I only set the calculator during interactive open, but works fine now that I make sure it's set on open and at run time:
from pyiron_atomistics import Project
pr = Project("tmp")
pr.remove_jobs(silently=True, recursive=True)
job = pr.create.job.Gpaw("gpaw", delete_existing_job=True)
job.input["encut"] = 100
job.input["kpoints"] = 3*[1]
s1 = pr.atomistics.structure.bulk("Al", cubic=True)
s2 = pr.atomistics.structure.bulk("Al", cubic=True)
s2.positions[0, 0] += 0.2
job.structure = s1
with job.interactive_open() as ijob:
ijob.run()
ijob.structure = s2
ijob.run()The Gpaw package is not part of the regular environment, but is there for notebooks, so I included this in the integration tests.
In the process I discovered that the AseJob class is actually abstract, only gets used for Gpaw, and is completely undocumented and untested. This displeases me, but is outside the scope of this PR.
In case you interactively opened and have since supplied a new structure.
Only in the integration tests, since we don't have the package in the main tests.
liamhuber
added
integration
E.g. for pyiron contrib protocols
|
The integration tests are shot straight to hell, but not the part from this PR. |
liamhuber
removed
the
integration
pyiron_atomistics/gpaw/pyiron_ase.py
Outdated
| @@ -26,7 +25,7 @@ | |||
| __date__ = "Sep 1, 2018" | |||
|
|
|||
|
|
|||
| class AseJob(GenericInteractive): | |||
| class AseJob(GenericInteractive, ABC): | |||
There was a problem hiding this comment.
Why is the AseJob now an abstract class? I thought the use case of this class is that ASE atoms which have a corresponding calculator object attached can be executed directly inside pyiron using this class.
There was a problem hiding this comment.
I made it an abstract class because it was only used as a parent to GpawJob and it had a method which, unless a user makes special modifications to a particular attribute using extra-pyiron tools, gets called and raises a NotImplementedError. That smells so much to me like an abstract class with an abstract method that I made it one.
You can't even pr.create.job this class.
To appease you I will remove the abstractness, but this class is terrible software.
I am not even sure if we want to support this syntax. the recommended syntax would be: self._job = pr.create.job.Gpaw(self._job_name)
self._job.server.run_mode = "interactive"
self._job.run() |
|
So to be fair, my memory is not very good, but I am nonetheless 90% certain that one of the original objectives of Anyhow, that is (a) in an entirely different repository, and (b) not what is being fixed here. The fundamental problem I have with HEAD is that for gpaw you need to call |
|
You can use |
And a user is not using it, a developer is inside a job class over on contrib. Again though, the point is that one should not have to call run before calling initialize. If you have a problem with the fine details of how the protocol jobs are implemented, please go raise an issue on contrib. I don't see any need to discuss the best way to set the interactive mode here. |
|
Actually, I just realized now there are tests so this usage is in this PR. Sorry, it is appropriate to discuss it here. And I still feel interactive open is the right way to do it. |
Here is the source: def interactive_open(self):
"""
Set the run mode to interactive.
This is the same as setting :attr:`.server.run_mode.interactive`.
Must be called before :meth:`.run()` is called.
"""
self.server.run_mode.interactive = True
return _WithInteractiveOpen(self)Literally all |
|
Integration test failures are unrelated, see #1085. That means the tests are passing. IMO this is ready to be merged. |
|
I'm not sure anymore whether we discussed it anywhere in writing but |
liamhuber
added
integration
Over in pyiron_contrib #694, @sinazadeh was running into trouble that the NEB protocol was breaking with Gpaw jobs. Looking into it, this is because the protocols explicitly call:
The current implementation of
AseJob.interactive_initialize_interfacecalls forself._structure.calc.set_label(self.working_directory + "/"), butself._structure.calcis only set inAseJob.run_if_interactive! Thus the protocols run into trouble by explicitly initializing the interface instead of just blindly jumping into a run.run_if_interactivecallsinteractive_initialize_interfaceif it hasn't been called already, some my hope is that we can simply move the setting ofself._structure.calcinto the initializer with zero side-effects. Certainly it makes more sense to me to do this in initialization rather than running, all else being equal.As discussed in #1066, I don't have Gpaw on my local machine right now, so I can't verify that this works beforehand. Looking at the
.ci_support/environment.ymlit also looks like we don't install Gpaw for the CI unit tests. And what's more, looking at the test suite itself, I don't see any tests for the interactive behaviour. Is there someone with GPaw access who can manually see if this change introduces any bugs?EDIT: Jan released an OSX-compliant version of gpaw on conda forge, so I'll just check it myself.
EDIT: Turns out you need it in both places, so that if you're running interactively and update to a new structure object you still guarantee a calculator.