🐛 TrajectoryData: Add pbc
#7079
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7079 +/- ##
==========================================
+ Coverage 79.61% 79.63% +0.03%
==========================================
Files 566 566
Lines 43546 43566 +20
==========================================
+ Hits 34663 34690 +27
+ Misses 8883 8876 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mbercx
force-pushed
the
fix/trajectory-pbc
branch
from
c263b2b to
ac70fb7
Compare
mbercx
force-pushed
the
fix/trajectory-pbc
branch
from
ac70fb7 to
d50cf17
Compare
mbercx
force-pushed
the
fix/trajectory-pbc
branch
2 times, most recently
from
16f0cc0 to
1af1745
Compare
|
|
||
| self._internal_validate(stepids, cells, symbols, positions, times, velocities) | ||
| # set symbols as attribute for easier querying | ||
| pbc = pbc or [True, True, True] |
There was a problem hiding this comment.
| pbc = pbc or [True, True, True] | |
| pbc = pbc or [True, True, True] |
This default makes me uneasy. Should we instead look at the cells and see if it is defined and non-zero?
There was a problem hiding this comment.
Sensible check! My MC3D bias is showing again ^^
There was a problem hiding this comment.
if it is defined and non-zero?
Can the cells be defined as zero? 🤔
There was a problem hiding this comment.
Do not know when I would use a structure without specifying cells, but agree to consider them False if not specified
There was a problem hiding this comment.
Apparently you can set e.g. cell = [[0, 0, 0], [0, 1, 0], [0, 0, 1]], but I'm not sure that's sensible? @danielhollas right now I've adapted the default to be [False, False, False] in case cells=None. Is that sufficient? Or what did you have in mind?
|
Thanks @mbercx seems to be fine! |
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
@mbercx thank you very much for working on this. I'd like to also try this out in my app to verify.
Also, can you please add test cases for the default PBC behaviour?
mbercx
force-pushed
the
fix/trajectory-pbc
branch
from
3b71180 to
4144eb6
Compare
|
Done! Check if you agree with the logic:
I've also added tests for all cases. Not sure if a deprecation makes too much sense long term, as I noted above we will replace the |
Currently, the `TrajectoryData` is not functional for structures that are not periodic, since their `pbc` property is not considered when using the `set_structurelist` method, which is also used in the constructor. Here we adapt the class to also accept the `pbc` input in the `set_trajectory` method. The input should be a list/tuple of length 3 with boolean values, which is validated in the `_internal_validate` method. It is also optional; in case it is not provided the current default behavior is maintained, i.e. the structure is assumed to be periodic. The `pbc` should also be the same for all structures in the trajectory, which is validated in the `set_structurelist` method. In line with the current approach for `symbols`, the `pbc` are stored in the attributes and a property is added. The current changes try to remain backwards-compatible for `get_step_data`, i.e. it now keeps on returning the same tuple of length 5. However, this does mean that the method does not return the `pbc`. For the `get_step_structure` method, we intead directly set the `pbc` from fhe property obtained from the attributes.
mbercx
force-pushed
the
fix/trajectory-pbc
branch
from
4144eb6 to
cdd3e3d
Compare
|
@danielhollas if you have a moment, could you have another look? We could still squeeze this into 2.7.2, since it's fixing a bug in my view. I want to avoid having this open too long and it falling through the cracks. 🙏 |
|
Yep, it's in my todo list for this week, thanks for the ping! FWIW: I am not sure this should go to the patch release: it's a non-trivial change that fixes a bug that's been there forever so I think it's okay for it to be released in minor version, as it is basically adding new capability (and changes the API). |
3 participants
Note
There is a lot of code debt in the
TrajectoryData, and the API is quite old and not optimal. Fixing all of that is beyond the scope of this PR, and probably we should redesign it inaiida-atomistic.Fixes #6376
Fixes #7037
Pinging @danielhollas, @rikigigi and @dbidoggia for review, since they raised the original issues.
The current changes try to remain backwards-compatible for
get_step_data, i.e. it now keeps on returning the same tuple of length 5. However, this does mean that the method does not return thepbc. For theget_step_structuremethod, I directly set thepbcfrom the property obtained from the attributes.