Correcting lll_reduce execution and allow setting ftol for termination searching making heterogeneous interfaces

[jinlhr542]

Why setting the two structures the same before finding the mappings between them?

[DanielYang59]

Thanks for catching this. However I don't think this should be removed, instead should put it after the following (I think I accidentally swapped their order in #3691 🙈, sorry):

pymatgen/pymatgen/core/surface.py

Line 1147 in d1fac20

mapping = lll_slab.lattice.find_mapping(slab.lattice)

As the following lines:

pymatgen/pymatgen/core/surface.py

Lines 1135 to 1137 in 9719845

	# Sanitize Slab (LLL reduction + site sorting + map frac_coords)
	lll_slab = struct.copy(sanitize=True)
	struct = lll_slab

Do more than just copying the struct (copy is not in-place), but sanitize it (note the sanitize=True):

pymatgen/pymatgen/core/structure.py

Lines 2287 to 2292 in 9719845

	sanitize (bool): If True, this method will return a sanitized
	structure. Sanitization performs a few things: (i) The sites are
	sorted by electronegativity, (ii) a LLL lattice reduction is
	carried out to obtain a relatively orthogonalized cell,
	(iii) all fractional coords for sites are mapped into the
	unit cell.

[DanielYang59]

@janosh Sorry this indeed looks like an accidental bug I introduced.

[jinlhr542]

Many thanks for the reviewing. I have swapped the order to rewrite struct into lll_reduced one after doing mapping.

I also added another property for the CoherentInterfaceBuilder class to allow to set the ftol searching for different terminations. Sometimes a very small ftol is needed to shift the termination into different atomic planes as shown here.

[jinlhr542]

Also added some features to allow 2D-identical terminations which might be 3D-non-identical to be saved as shown in the examples below:




As can be seen, the first and second terminations will be merged into one in the old version. I added an extra index before the label of the slab termination so that they can be distinguished to avoid merging.

[DanielYang59]

First I need to make clear that I'm not a maintainer (just happen to contribute to this module). Some preliminary comments on this change to label_termination:

• I would be on board for giving this flexibility to control the clustering tolerance here, instead of hard-coding the tolerance to be 0.25.
• Can we possibly update the docstring to include these two new arguments here?
• We could (and should) make it backwards compatible (make this prefix an empty string by default for example).

[jinlhr542]

Many thanks for the comment. I have revised the code to make backwards usage compatible by setting up a label_index bool variable. I also updated the docstring.

[DanielYang59]

No problem.

• Instead of adding two args for a single behavior, perhaps we could reused the single t_index arg, and set it to None by default. And add a prefix only when t_index is not None.

  def label_termination(
      slab: Structure, 
  -    t_index=1, 
      ftol: float = 0.25, 
  -    label_index=False,
  +    t_index=None, 
  ) -> str:
      if t_index is None:
          return ... 
      else:
          return ...

  Though these two args should be merged in this case, another personal preference being: it might be good to have two functional-related args close to each other. i.e.:

  def label_termination(
      slab: Structure, 
  -    t_index=1, 
      ftol: float = 0.25, 
      label_index=False,
  +    t_index=1, 
  ) -> str:

  Now that label_index and t_index together control the same behavior, there is no reason to separate them (at least in this case).

• Can we update the docstring of label_termination to include the t_index arg?

• It might be very confusing to offset the index when it is passed directly:

    def label_termination(
        slab: Structure, 
        t_index=None, 
        ftol: float = 0.25, 
    ) -> str:
  -       return f"{t_index+1}_{form}_{sp_symbol}_{len(top_plane)}" 
  +       return f"{t_index}_{form}_{sp_symbol}_{len(top_plane)}" 

  I assume there is no good reason to use t_index + 1 here?

[jinlhr542]

Done. t_index + 1was used because I prefer to label the termination to start from 1. Indeed it is not necessary.

[DanielYang59]

Done.

Beautiful! Thanks for the change.

The docstring of function label_termination is still missing?

t_index + 1was used because I prefer to label the termination to start from 1. Indeed it is not necessary.

Even you prefer the index to start from 1, adding an offset inside label_termination might still be unexpected (as you pass index in, and index + 1 comes out).

I'm not suggesting you do this here, but if you really want the index counter to start from a value other than zero, you could use the start arg of enumerate, e.g. using enumerate(iterable, start=1) would start the counter from 1 instead of 0.

Another tip being, you could install pre-commit locally (pre-commit install) to run pre-commit hooks prior to each commit.

[jinlhr542]

Sorry for late updating. I have revised again according to your suggestion and added docstring for label_termination()

[DanielYang59]

It appears the docstring of this func is still missing.

Also can you please add type annotation for t_index as well? (t_index: int | None = None)

You could add an extra , after t_index to wrap this def into multiple lines now that it's getting very long:

- 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:

And ruff would automatically wrap it.

[jinlhr542]

Dear developers it seems there are some conflicts cannot be resolved automatically, could you please have a check?

[DanielYang59]

Sorry I'm unable to resolve conflicts for you as I don't have write access.

You might want to look at this tutorial to resolve merge conflicts with VSCode with a GUI, and this from the web.

@janosh Can you please review this?

[jinlhr542]

It seems that I do not have write access to resolve the conflicts on the web. I am using the github desktop and I have resolved it locally before commiting this version. I have no idea why the check still reported conflicts...

[DanielYang59]

I didn't see any merge conflicts from my side for some reason, when you merge (and resolve conflicts during merge), you would also need to push the merge commit (just like any other commit). I didn't see the merge being pushed, can you please confirm this?

Also your fork is not up to date with remote, perhaps you should consider syncing it?

[jinlhr542]

Actually I have solved the conflict from my side before push it. I clicked syncing just now but it also says conflict remaining