Fix calculate static state after relax #338
Conversation
|
|
||
| _atom_attributes = state._atom_attributes | {"forces"} # noqa: SLF001 | ||
| _system_attributes = state._system_attributes | { # noqa: SLF001 | ||
| _atom_attributes = SimState._atom_attributes | {"forces"} # noqa: SLF001 |
There was a problem hiding this comment.
I don't like changing the atom attributes from state to SimState. But I'm okay with it for now since torchsim currently only supports calculating energies, forces, and stresses.
When we support magnetic moments (and arbitrary properties in general) we will need to rethink how to create this
There was a problem hiding this comment.
The reason why I needed to set these atom attributes to be derived from SimState is because if I left it as state, then inside the trajectory reporter, it would think that StaticState would have extra attributes (that we did not initialize when we created it) the extra attribute in particular is velocity.
So by explicitly defining the _atom_attributes to be from SimState, we only say that this StaticState has a fixed limited set of attributes
There was a problem hiding this comment.
I plan to add mag moms soon so I'd prefer we pick a solution here that will be generalizable.
There was a problem hiding this comment.
Yeah I need to think through that some more as well. On another note, I'd prefer to do that in a different PR so this one can be constrained to fixing just this bug
curtischong
force-pushed
the
fix-static-state-after-relax
branch
2 times, most recently
from
6eca84b to
50d24c7
Compare
… a test to document the fix you can now calculate static properties after optimizing better docs fix prek
curtischong
force-pushed
the
fix-static-state-after-relax
branch
from
35a5619 to
5cb88ea
Compare
thomasloux
left a comment
thomasloux
left a comment
There was a problem hiding this comment.
agreed with the change, it's what we do in all other subclass of SimState.
3 participants
Summary
Before this change, we'd get this error
This is because we were creating a static_state but passing in fields that already existed (because we optimized the state before calculating the static property). Since we used **vars() to initialize the static state, we'd pass properties like energies twice into the constructor for StaticState.
To fix this PR I also had to make StaticState's attributes be dependent on only SimState's attributes.
Before a pull request can be merged, the following items must be checked: