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.51% 77.87% +0.37%
==========================================
Files 560 567 +7
Lines 41444 42171 +727
==========================================
+ Hits 32120 32837 +717
- Misses 9324 9334 +10 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
| 'An instance of StructureData should be passed to ' 'the KpointsData, found instead {}'.format( | ||
| structuredata.__class__ | ||
| ) | ||
| error_message = ( |
There was a problem hiding this comment.
I think this can directly move to raise exception?
There was a problem hiding this comment.
see #6632 (comment)
| else: | ||
| raise ValueError(error_message) |
There was a problem hiding this comment.
Maybe it is possible to move the exception to same condition and move the set_cell to same condition.
There was a problem hiding this comment.
Hi @unkcpz, thank you for the comment. I have actually another idea, which is replacing the whole block from line 228 to 245 with:
if has_atomistic():
from aiida_atomistic import StructureData as AtomisticStructureData
if not isinstance(structuredata, StructureData) and not isinstance(structuredata, AtomisticStructureData if has_atomistic() else StructureData):
raise ValueError('An instance of StructureData or aiida-atomistic StructureData should be passed to '
'the KpointsData, found instead {}'.format(structuredata.__class__))
else:
cell = structuredata.cell
self.set_cell(cell, structuredata.pbc)This seems redundant, but actually makes more compact the check. What do you think?
There was a problem hiding this comment.
I guess if the structuredata is neither StructureData nor the AtomisticStructureData, the else block of calling structuredata.cell and self.set_cell with structuredata.pbc will fail?
If so, why not use try..except block and put the current else block in the try and the catch the exceptions for correspond operations? Let me know if I am missing something and we can try it together in person.
There was a problem hiding this comment.
In the way you suggest, if structuredata was an arbitrary object which defines cell and pbc as attributes, the exceptions will not be raised but it should. And the user will not see the error (i.e. not providing an orm or atomistic StructureData).
Anyway, let's discuss this in person on monday
for more information, see https://pre-commit.ci
orm.StructureDatahas ato_atomisticmethod, and anhas_atomisticfunction is implementedset_cell_from_structuremethod forKpointsDataaiida-atomisticis not installed (skip_atomisticfixture)