Allow for setting termination_ftol for film and substrate slabs separately#4498
Allow for setting termination_ftol for film and substrate slabs separately#4498shyuep merged 17 commits intomaterialsproject:masterfrom
Conversation
|
@DanielYang59 Dear Daniel, could you please have a look at this PR? |
DanielYang59
left a comment
There was a problem hiding this comment.
Hi thanks for asking me, I'm not a maintainer so here is my personal opinions and you could decide on your own
| substrate_miller (tuple[int, int, int]): miller index for the substrate layer | ||
| zslgen (ZSLGenerator | None): BiDirectionalZSL if you want custom lattice matching tolerances for coherency. | ||
| termination_ftol (float): tolerance to distinguish different terminating atomic planes. | ||
| termination_ftol (float): tolerances (film, substrate) to distinguish different terminating atomic planes. |
There was a problem hiding this comment.
I'm not very familiar with the use case of CoherentInterfaceBuilder but I assume it's reasonable to allow more fine-grained control over the film and substrate. In this case I guess: either you allow termination_ftol to be float | tuple[float, float] but you should update the docstring accordingly (currently it's still float), or you could deprecate this arg with two args like film/substrate_termination_tol.
|
|
||
| film_slabs = film_sg.get_slabs(ftol=self.termination_ftol, filter_out_sym_slabs=self.filter_out_sym_slabs) | ||
| sub_slabs = sub_sg.get_slabs(ftol=self.termination_ftol, filter_out_sym_slabs=self.filter_out_sym_slabs) | ||
| if type(self.termination_ftol) is not tuple: |
There was a problem hiding this comment.
should use isinstance here as type() is would only allow exact tuple but not subclasses
| film_slabs = film_sg.get_slabs(ftol=self.termination_ftol, filter_out_sym_slabs=self.filter_out_sym_slabs) | ||
| sub_slabs = sub_sg.get_slabs(ftol=self.termination_ftol, filter_out_sym_slabs=self.filter_out_sym_slabs) | ||
| if type(self.termination_ftol) is not tuple: | ||
| self.termination_ftol = (self.termination_ftol, self.termination_ftol) |
There was a problem hiding this comment.
I guess it's more readable to unpack within __init__ instead of accessing with index, something like:
if isinstance(termination_ftol, tuple):
self.film_termination_ftol, self.substrate_termination_ftol = termination_ftol
else:
self.film_termination_ftol = self.substrate_termination_ftol = termination_ftolThere was a problem hiding this comment.
Daniel, many thanks for these helpful suggestions. I have commited these changes.
|
Thanks. |
Summary
This pull request is to alllow for setting termination_ftol for film and substrate slabs separately.
The original CoherentInterfaceBuilder of the pymatgen.analysis.interface module has a parameter termination_ftol specifying the tolerance distance clustering different termination atomic planes according to their off-plane position. This value is applied for both the film and substrate slabs. However, since film and substrate have different structures, it is useful to allow for setting this value separately for film and substrate.
The test script is also updated.