Allow for setting termination_ftol for film and substrate slabs separately #4498
Conversation
|
@DanielYang59 Dear Daniel, could you please have a look at this PR? |
| @@ -43,7 +43,7 @@ def __init__( | |||
| film_miller (tuple[int, int, int]): miller index for the film layer | |||
| 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.
| @@ -133,24 +133,29 @@ def _find_terminations(self): | |||
| reorient_lattice=False, # This is necessary to not screw up the lattice | |||
| ) | |||
|
|
|||
| 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.
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.