Separate module for output classes #123
Conversation
Now we can always just use the class directly
This also removes the misdirection layer
There was a problem hiding this comment.
I am definitely happy to move the classes out to their own module and to use it for calc static.
I found this PR to be too verbose, although I think I'm ultimately to blame for this since I introduced __call__, which required actually instantiating these classes. I stacked a new PR that replaces __call__ with a class method get which behaves the same way but is accessible right from the class. This lets use remove the get_* layer of misdirection functions entirely. As a result, I also directly leverage .fields() since I no longer instantiate the classes.
Not addressed in my PR, but something we should think about, is the way the MD output doesn't inherit from the static output. I would suggest renaming LammpsStaticOutput.energy to LammpsStaticOutput.energy_pot, and then using LammpsMDOutput(LammpsStaticOutput) directly. This has the additional effect that stresses can be calcualted in MD -- is this possible? Anyhow, I would like that direction but thought it warranted a second thought before I stacked it on. I suspect @samwaseda will have an opinion on this.
There was a problem hiding this comment.
With the changes merged in from my stacked PR, I'm happy. I do think we need to address energy vs energy_pot and would be inclined to get @samwaseda's opinion before merging, but IMO we could merge this and then open an issue on that topic to fix later.
@jan-janssen, now that #129 is open I feel we have cover to merge #121 as well if you want -- with the issue there we won't lose track of its deficiencies, and it's only imperfect not broken. I don't think we have to merge it, but I'm about to be literally unconscious for ~7 hours so I just want you to know I'm not standing in the way 😂 |
I agree, I just want to give Sam the chance to have a look at it, once he agrees we can merge it. |
No description provided.