Providing support for atomistic StructureData #6632
Conversation
- orm.StructureData `to_atomistic` method and `has_atomistic` function - adapted `set_cell_from_structure` method for KpointsData - added tests for both structure and kpoints.
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6632 +/- ##
==========================================
- Coverage 77.92% 77.90% -0.01%
==========================================
Files 563 563
Lines 41671 41702 +31
==========================================
+ Hits 32467 32483 +16
- Misses 9204 9219 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @mikibonacci!
The goal of the PR is quite clear.
I have some requests on the verbose if..else conditions of array/kpoints.py which a bit unreadable. I'd suggest if the operation of different conditions are the same, it might be clear to combine conditions using or?
The tests seems can be improved by creating an atomic lib structure fixture to be used for multiple tests.
for more information, see https://pre-commit.ci
tests/orm/nodes/data/test_kpoints.py
Outdated
| from aiida_atomistic import StructureData, StructureDataMutable | ||
|
|
||
| alat = 5.430 # angstrom | ||
| cell = [ | ||
| [ | ||
| 0.5 * alat, | ||
| 0.5 * alat, | ||
| 0.0, | ||
| ], | ||
| [ | ||
| 0.0, | ||
| 0.5 * alat, | ||
| 0.5 * alat, | ||
| ], | ||
| [0.5 * alat, 0.0, 0.5 * alat], | ||
| ] | ||
| self.alat = alat | ||
| mutable = StructureDataMutable() | ||
| mutable.set_cell(cell) | ||
| mutable.add_atom(positions=(0.000 * alat, 0.000 * alat, 0.000 * alat), symbols='Si') | ||
| mutable.add_atom(positions=(0.250 * alat, 0.250 * alat, 0.250 * alat), symbols='Si') | ||
| self.structure = StructureData.from_mutable(mutable) |
There was a problem hiding this comment.
This can be moved as a generate_atomistic_structure_data() when called it return this example structure.
I think there is also quite a bit duplicated code between TestKpoints and TestKpointsAtomisticStructureData. My suggestion is to combine these to above and use https://docs.pytest.org/en/stable/how-to/parametrize.html that same test logic works for both of them.
both in kpoints.py and test_kpoints.py
for more information, see https://pre-commit.ci
|
Hi @unkcpz , I pushed some fix following your suggestion, thanks! To give a comment, I used the pytest parameterization in the test_kpoints.py, so we don't have code duplication. I am not sure it is really a 100% parametrization, in the sense that in the new generate_structure (the old init_profile), I put a check for the structure data type, but in this way we don't need to implement a new method to generate the atomistic version. For sure this will change when the orm.StructureData will be removed. If anything else, please let me know :) Thanks! |
To be implemented soon, to make bands.py work.
for more information, see https://pre-commit.ci
|
Hi @unkcpz , I think the PR is ready, I should have added all you requested changes. But let me know if I forgot something, thanks! |
There was a problem hiding this comment.
Thanks @mikibonacci, all good! can you fix the docstring and then we can get it merged.
The doc build failed because of:
�[2Kreading sources... [100%] tutorials/index
/home/docs/checkouts/readthedocs.org/user_builds/aiida-core/envs/6632/lib/python3.11/site-packages/aiida/orm/nodes/data/structure.py:docstring of aiida.orm.nodes.data.structure.has_atomistic:2: WARNING: Field list ends without a blank line; unexpected unindent.
looking for now-outdated files... none found
| @@ -223,16 +223,24 @@ def set_cell_from_structure(self, structuredata): | |||
|
|
|||
| :param structuredata: an instance of StructureData | |||
| """ | |||
| from aiida.orm import StructureData | |||
| from aiida.orm.nodes.data.structure import StructureData as LegacyStructureData | |||
There was a problem hiding this comment.
LegacyStructureData, I like the name 👍
orm.StructureDatahas ato_atomisticmethod, and anhas_atomisticfunction is implementedset_cell_from_structuremethod forKpointsDataaiida-atomisticis not installed (skip_atomisticfixture)