Fix lll_reduce for slab generation#3927
Conversation
This reverts commit 70628b4.
1. fixing problem for the lll_reduce process when making slabs, doing mapping before updating the structure 2. allow to set ftol of the termination distances for hierarchical cluster so that some non-identical terminations close to each other can be identified 3. allow to add index for terminations so that terminations with the same space group can be distinguished
|
@DanielYang59 Sorry, it seems that my changes have conflicts with the recent updates, I remake another pull request using the recent version |
|
I understand merge conflicts could be confusing but don't worry. Merge conflicts are common things that would happen when you and another person try to make changes to the same line of code. When such things happen, you just need to choose whatever conflict resolver that works best for you (GUI/CLI), and decide what the final conflicted line should end up by yourself. In most cases, reopen a PR is not necessary, perhaps you could try to resolve them next time when such things happen. I would ping @janosh for review as I'm not a maintainer and therefore don't have write access to the repo. Can I ask for your patience as he has been quite busy recently? Thank you for your effort towards correcting and improving the code. |
|
Many thanks for the suggestion... I have another issue here. I found that installing the package and by |
That's exactly right observation. By Meanwhile, if you were trying to contribute to the code, you might want to use
I didn't manage to recreate this issue from my side. Perhaps you should double check if you have a file called |
|
Thank you for the explanation but I found the that the reason might be the package is not fully installed, this is the pymatgen folder installed using conda environment... As you can see most of the modules were not successfully installed |
|
It's weird, but it's hard to give any advice without seeing the log. Can you show me the log from |
Indeed there is an issue of the emmet-core dependency problem but it finally says successfully installed... |
|
I believe it's better not to trust an incompletely resolved package dependency? |
|
yes, I found the problem. I am using mp-api which requires emmet-core, and emmet-core requires lower version of pymatgen... In this way I can only use the old version pymatgen now... Many thanks for your help there |
e3fbc67 to
41e6d99
Compare
janosh
left a comment
There was a problem hiding this comment.
@jinlhr542 thanks for these improvements! could you add unit tests for the keywords you added?
…tions for making interface Interfaces made by identical slabs can be non-identical because the relative transformation of the misorientation and the termination variation do not ensure symmetry, especially when the film and substrate have different point groups. Therefore, the termination finding function should allow to generate all the possible terminations. This can help others to develop more robust algorithm to group the equivalent interfaces made by different terminations.
Could you please try this test? import unittest
from pymatgen.core.structure import Structure
from pymatgen.core.lattice import Lattice
from pymatgen.analysis.interfaces import CoherentInterfaceBuilder, SubstrateAnalyzer
class TestCoherentInterfaceBuilder(unittest.TestCase):
def setUp(self):
#build substrate & film structure
basis = [[0, 0, 0], [0.25, 0.25, 0.25]]
self.substrate = Structure(
Lattice.cubic(a=5.431),
["Si", "Si"],
basis)
self.film = substrate = Structure(
Lattice.cubic(a=5.658),
["Ge", "Ge"],
basis)
def test_termination_searching(self):
sub_analyzer = SubstrateAnalyzer()
matches = list(sub_analyzer.calculate(substrate = self.substrate, film = self.film))
cib = CoherentInterfaceBuilder(film_structure = self.film,
substrate_structure=self.substrate,
film_miller=matches[0].film_miller,
substrate_miller=matches[0].substrate_miller,
zslgen=sub_analyzer,termination_ftol=1e-4,label_index=True,\
filting_out_sym_slabs=False)
self.assertTrue(cib.terminations == [('1_Ge_P4/mmm_1', '1_Si_P4/mmm_1'),\
('1_Ge_P4/mmm_1', '2_Si_P4/mmm_1'),\
('2_Ge_P4/mmm_1', '1_Si_P4/mmm_1'),\
('2_Ge_P4/mmm_1', '2_Si_P4/mmm_1')], \
"""
termination results wrong; the correct list should be:
[('1_Ge_P4/mmm_1', '1_Si_P4/mmm_1'),
('1_Ge_P4/mmm_1', '2_Si_P4/mmm_1'),
('2_Ge_P4/mmm_1', '1_Si_P4/mmm_1'),
('2_Ge_P4/mmm_1', '2_Si_P4/mmm_1')].
""")
if __name__ == "__main__":
unittest.main() |
Dear Janosh Could you please check the commited tests? It will be appreciated if this pull request can be accepted soon. |
janosh
left a comment
There was a problem hiding this comment.
minor variable rename suggestions
|
|
||
| def label_termination(slab: Structure) -> str: | ||
| """Label the slab surface termination.""" | ||
| def label_termination(slab: Structure, ftol: float = 0.25, t_index: int | None = None) -> str: |
There was a problem hiding this comment.
how about just tol instead of ftol? does the f have any meaning? also, how about t_index->t_idx for brevity?
| def label_termination(slab: Structure, ftol: float = 0.25, t_index: int | None = None) -> str: | |
| def label_termination(slab: Structure, ftol: float = 0.25, t_index: int | None = None) -> str: |
There was a problem hiding this comment.
f means flat cluster, to be consistent with the ftol argument of the get_slabs() function in the SlabGenerator class
Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com> Signed-off-by: Jason Xie <jasonxie@sz.tsinghua.edu.cn>
Modified accordingly, please review again. |
| filter_out_sym_slabs: whether to filter out identical slabs with different termination, | ||
| this might need to be set as False to find more non-identical terminations because slab | ||
| identity separately dose not mean combinational identity. | ||
| """ |
There was a problem hiding this comment.
can you clarify/rephrase "identity separately does not mean combinational identity". combinational is a rare word. do you mean identity of slab + termination?
There was a problem hiding this comment.
Specifically, for two combinations (A1, B1) and (A2, B2). 'A1 equal to A2' and 'B1 equal to B2' does not always make (A1, B2) equal to (A2, B2). When making two interfaces if the two set of slabs are related by certain symmetry operations respectively, these operations need to be compatible to make a 'combinational' identity thus identical interfaces.
lll_reduce for slab generation
Signed-off-by: Jason Xie <393106066@qq.com>
|
@jinlhr542 Great work! |
By making the termination identification process for interface generation more robust.