Add Structure.sublattices and class RandomStructureTransformation to standard_transformations.py#3057
Add Structure.sublattices and class RandomStructureTransformation to standard_transformations.py#3057exenGT wants to merge 28 commits intomaterialsproject:masterfrom
Structure.sublattices and class RandomStructureTransformation to standard_transformations.py#3057Conversation
…tgen/transformations/standard_transformations.py
|
Thanks @exenGT, I added a question to the associated issue. |
|
@janosh All your suggestions have been addressed in my latest commits. |
janosh
changed the title
sublattices to pymatgen/core/structure.py; adding class RandomStructureTransformation to pymatgen/transformations/standard_transformations.pyStructure.sublattices and class RandomStructureTransformation to standard_transformations.py
janosh
left a comment
janosh
left a comment
There was a problem hiding this comment.
@exenGT Looking good! Could you add a test in test_standard_transformations.py?
| subl = structure.sublattices | ||
|
|
||
| # fill the sublattice sites with pure-element atoms | ||
| self.all_structures = [] |
There was a problem hiding this comment.
is it necessary to store the structures on the transformation instance? might balloon mem usage if people create many of these RandomStructureTransformations.
There was a problem hiding this comment.
I used this syntax because there is an is_one_to_many property in the class; should it be False by default? (I see it is True for other classes)
There was a problem hiding this comment.
I think there's no connection between is_one_to_many (that part is good) and storing the transformed structures on the transformation instance?
There was a problem hiding this comment.
Could you elaborate more on this? I feel I'm not totally understanding. For example, the apply_transformation method of OrderDisorderedTransformation returns a list of structures as well (self._all_structures). What is the issue with this approach?
There was a problem hiding this comment.
Could you explain the need to store all structures for later? Why not just return them and let them be garbage collected if they go out of scope in the user's code? Currently they live as long as the Transformation instance which is not great given structures are notoriously mem hungry.
There was a problem hiding this comment.
I see. This feature is here simply because I'm copying what's been written for the other transformations. If you think it's not a good approach, then I can modify it in my code accordingly.
janosh
added
enhancement
exenGT
force-pushed
the
dev
branch
5 times, most recently
from
6d162f3 to
84f7832
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3057 +/- ##
==========================================
- Coverage 74.69% 74.12% -0.57%
==========================================
Files 230 230
Lines 69375 69419 +44
Branches 16154 16163 +9
==========================================
- Hits 51818 51460 -358
- Misses 14490 14923 +433
+ Partials 3067 3036 -31
☔ View full report in Codecov by Sentry. |
…round in apply_transformation() of class RandomStructureTransformation.
…) in class Trajectory.
janosh
force-pushed
the
master
branch
2 times, most recently
from
3c23114 to
36e289c
Compare
janosh
force-pushed
the
dev
branch
3 times, most recently
from
9be5122 to
e32fd8f
Compare
janosh
force-pushed
the
master
branch
4 times, most recently
from
d725325 to
dca98be
Compare
janosh
force-pushed
the
master
branch
2 times, most recently
from
e3fbc67 to
41e6d99
Compare
4 participants
Closes #3049.