Partial periodic boundary conditions

[gpetretto]

Summary

As a follow up to #2364 I am opening this PR to discuss about the introduction of partial periodic boundary conditions (PPBC).

Details

This first draft is based on these main points:

• PPBC are introduced at the level of Lattice with a tuple of three bool in the pbc attribute, defining the periodic boundary conditions along the three edges of the lattice.
• Objects having a Lattice get their periodic boundary conditions from it. This avoids inconsistencies among different objects. i.e. PeriodicSite and Structure have the same reference to define the PBC.
• Molecule and Structure stay on two different branches of the inheritance tree starting from SiteCollection, even though Molecule could be thought of a subset of the case where pbc=(False, False, False).
• fractional coordinates remain the main reference for PeriodicSite. This ensures the backward compatibility with the serialised data and avoids the ambiguity of what frac_coords would mean in PPBC. This implies that a somewhat meaningful 3D Lattice should be defined even with PPBC.

Based on these, the changes required to the code are relative small and seems that backward compatibility could be easily preserved (at least, all the tests pass).

In practice, the main changes are the addition of the pbc attribute and the updates to take it into account in functions related to distances and where to_unit_cell is True. Since distances are calculated in a few key functions, the changes should propagate to all functions that rely on these.
Being a first draft I did not add any test and changed the docstrings in just a few places. I also did not run the checks for the coding style.
I am not used to write cython code, so some additions in coord_cython.pyx could probably be optimised. Also, the standard implementation of pbc_shortest_vectors relies on the LLL reduced lattice and makes it incompatible with the pbc. Is there a specific reason for that, aside from the use of the lll_frac_tol argument? I switched to the standard lattice if the crystal is not 3D periodic.

Example

coords = [[0,0,0],
          [0.9, 0.9, 0.9]]
l = Lattice(np.eye(3) * [1,2,3], pbc=(True, False, True))
s = Structure(lattice=l, coords=coords, species=["Si"]*len(coords))
print(s.distance_matrix[0, 1])

gives 1.82756669, instead of 0.37416574 with pbc=(True, True, True).

Comments

Assuming that the main points listed above are fine, here are some more considerations.

It would be relevant to to decide if this new feature should stay directly in Structure (as implemented here) or a new intermediate class should be introduced between SiteCollection and Structure, with Structure having pbc=(True, True, True) fixed.
If the PPBC stay in Structure, how to handle its subclasses (e.g. Slab, Interface, SymmetrizedStructure)? I suppose that for most of them PPBC would not make sense.
How should all the objects that accept a Structure behave? For example running the SpacegroupAnalyzer should not be meaningful for PPBC. Should these make checks on pbc and raise?
Could objects like the StructureMatcher be adapted to work with PPBC?

Concering Lattice:

• should the pbc attribute be immutable?
• should pbc=(False, False, False) be allowed?
• some methods may be considered ambiguous. Methods like get_niggli_reduced_lattice, get_wigner_seitz_cell will keep working while ignoring pbc, but are likely to return inconsistent results for PPBC. Should they raise an error? Methods like scale should still act on the whole lattice, or modify only the periodic directions?

Concerning Structure:

• some methods like get_primitive_structure, get_orderings could probably be adapted to work with PPBC, but I suppose it will not be straightforward. Should this be done?
• Should the writing to a file be allowed in PPBC as if it was a normal Structure? Are there file formats that support PPBC?
• how should a Structure with PPBC be scaled? (e.g. __mul__)

While it would not necessarily be the best option, I would say that allowing PPBC only in a new class, parent of Structure, should avoid easily all the problems related to checks in cases where a fully periodic Structure is expected.

[coveralls]

Coverage decreased (-0.7%) to 83.291% when pulling 9dfe166 on gpetretto:ppbc into 6bbe18e on materialsproject:master.

[mkhorton]

Thanks for this PR @gpetretto! I appreciate the write-up and detailed thoughts, I'll take some time to read it over and think about the concerns raised.

[gpetretto]

Hi @mkhorton, sorry for the bothering, but do you have any update on this?

[mkhorton]

Thanks for your patience @gpetretto and thank you for the reminder (it was necessary! apologies, been busy recently).

I've had a look over the PR now and it looks good to me. I like this approach overall. I'm not familiar enough to guarantee the Cython code is correct so I will trust you and the tests on this.

Do you have any remaining concerns?

[gpetretto]

Thanks for the reply @mkhorton. Just to be sure, do you mean that the comments in the opening post do not need further discussion and things can stay roughly as they are in the current PR?
If that is the case, should I just add the tests and finalize the PR?

[mkhorton]

Yes, I would agree with finalizing the PR and using this approach. If anyone disagrees however or has further input, now is the time to comment!

[gpetretto]

I fixed a few points and added tests. If there are no other comments this sohuld be ready for a review and merge.

[shyuep]

Why is there a need to use None here? We can just default to (True, True, True)? This is a tuple, which is not mutable. It is allowed as an arg.

[gpetretto]

Indeed. I think I did it because it was easier to avoid issues with some tests failing during the first draft. Then I didn't think of changing it. It should be fixed now

[shyuep]

This is not needed.

[shyuep]

Same comment as the __init__.

[shyuep]

Thanks