Define output classes based on instances rather than classes #186
Conversation
jan-janssen
added
the
format_black
There was a problem hiding this comment.
I mean, sure, but... why? Please include a description in the PR to clarify your intent.
I suspect this is just for readability? In that case I disagree and like it better the old way, but I don't actually object and if this is the style you prefer I'm A-OK with that. Before and after are both reasonable.
| ASEOutputMolecularDynamics = OutputMolecularDynamics( | ||
| **{k: getattr(ASEExecutor, k) for k in OutputMolecularDynamics.keys()} | ||
| ) | ||
| cache = {q: [] for q in output_keys} | ||
| for i in range(int(run / thermo)): | ||
| dyn.run(thermo) | ||
| calc_dict = ASEOutputMolecularDynamics.get( | ||
| ASEExecutor(ase_structure=structure, ase_calculator=ase_calculator), | ||
| *output_keys, | ||
| ase_instance = ASEExecutor( | ||
| ase_structure=structure, ase_calculator=ase_calculator | ||
| ) | ||
| calc_dict = OutputMolecularDynamics( | ||
| **{k: getattr(ase_instance, k) for k in OutputMolecularDynamics.keys()} | ||
| ).get(*output_keys) |
There was a problem hiding this comment.
This change is different, as it actually removes an intermediately defined class. I'm 100% behind this change.
There was a problem hiding this comment.
But this change requires changing the other calls as well. The important part is the change in output from:
def get(self, engine, *output: str) -> dict:
return {q: getattr(self, q)(engine) for q in output}
To:
def get(self, *output: str) -> dict:
return {q: getattr(self, q)() for q in output}
By no longer providing the engine as parameter we can simplify the code as illustrated by the ASE example above. But this affects all output classes at the same time, so all output classes need to be updated.
There was a problem hiding this comment.
Ahh, ok, I had totally failed to grok the change in Output, I apologize. A version of the final sentence there would be perfect as the PR description.
Ok, then I am not sold on introducing variables to hold the XProperties rather than using them in-line, but I also don't object to it. And I like the simplification of the Output initialization/get, that's a nice change.
No description provided.