Lattice is now owning the boundary conditions #629

zerothi · 2023-10-20T17:17:48Z

This should be a first step towards moving the
nsc attribute from Lattice to somewhere more
appropriate.

This also makes the boundary conditions in the
Grid more manageable since it is the lattice which keeps the information.

Simplified reading xyz files since the boundary
conditions are now explicit.

@tfrederiksen and @pfebrer a review please. :)

Closes Lattice.boundary_cond boundary conditions for the lattice #626 Default Lattice.nsc and relation to periodic/nonperiodic directions #625
Tests added
Ranned isort . at top--level
Documentation for functionality, docs/
Changes documented, CHANGELOG.md

A small snippet showing its use:

from sisl import BoundaryCondition as BC, Lattice

print("Available boundary conditions:")
for b in BC:
    print("  ", b)

# Define a lattice
print(Lattice(1, boundary_condition=BC.PERIODIC))
print(Lattice(1, boundary_condition=[BC.DIRICHLET, BC.NEUMANN, BC.PERIODIC]))
print(Lattice(1, boundary_condition=[BC.DIRICHLET, [BC.NEUMANN, BC.DIRICHLET], BC.PERIODIC]))


print("Setting new boundary conditions:")
latt = Lattice(1, boundary_condition=BC.PERIODIC)
print(latt)
latt.set_boundary_condition(c=[BC.NEUMANN, BC.DIRICHLET])
print(latt)
latt.set_boundary_condition(BC.PERIODIC)

print("Printing stuff")
print(latt.boundary_condition)
print(latt.pbc)

# doing
latt.boundary_condition = ...
# is equivalent to latt.set_boundary_condition

This should be a first step towards moving the nsc attribute from Lattice to somewhere more appropriate. This also makes the boundary conditions in the Grid more manageable since it is the lattice which keeps the information. Simplified reading xyz files since the boundary conditions are now explicit. Signed-off-by: Nick Papior <[email protected]>

src/sisl/lattice.py

src/sisl/grid.py

@@ -14,8 +15,8 @@
 from ._help import dtype_complex_to_real, wrap_filterwarnings
 from ._internal import set_module
 from .geometry import Geometry
-from .lattice import LatticeChild
-from .messages import SislError, deprecate_argument
+from .lattice import BoundaryCondition, LatticeChild


src/sisl/grid.py

@@ -30,6 +31,10 @@

 __all__ = ['Grid', 'sgrid']

+_log = logging.getLogger("sisl")
+_log.info(f"adding logger: {__name__}")
+_log = logging.getLogger(__name__)


src/sisl/lattice.py


    def __repr__(self):
        a, b, c, alpha, beta, gamma = map(lambda r: round(r, 4), self.parameters())
-        return f"<{self.__module__}.{self.__class__.__name__} a={a}, b={b}, c={c}, α={alpha}, β={beta}, γ={gamma}, nsc={self.nsc}>"
+        BC = BoundaryCondition
+        bc = self.boundary_condition


zerothi · 2023-10-20T19:11:40Z

Added a small code-snippet in the top post.

pfebrer · 2023-10-20T20:50:08Z

Great!

Some comments:

Don't you think that bc has a clear enough meaning for a lattice? boundary_condition seems too long to me.
Could the argument also support strings? In general I feel like enums are not very user friendly because you need to import them, which is tedious.
Could it make sense that there was also a set_pbc method? With an argument that could be True or False (False could mean to set open boundary conditions, I think this is what most users would expect).
Why is BoundaryConditionType int instead of the enum itself?

zerothi · 2023-10-21T09:26:10Z

I partially agree. What does Thomas say?
Good idea. I'll amend
I am not particularly fond of that doing the other would still be good. I'll see if bool can be translated to periodic.
It is because it is contained in the numpy array, perhaps this is overkill, so we might just change it. Any pros/cons on str vs int?

zerothi · 2023-10-22T19:14:04Z

@pfebrer oh now I see your point in 4. I'll amend.

tfrederiksen · 2023-10-23T07:54:04Z

I think this looks good. Just some simple observations:

boundary_condition=True is accepted, but is this intended?

>>> sisl.Lattice(1, boundary_condition=True)
<sisl.Lattice a=1.0, b=1.0, c=1.0, α=90.0, β=90.0, γ=90.0, bc=[U, U, U], nsc=[1 1 1]>

Would it make sense to set the z-direction to be nonperiodic in the 2D geometries?

>>> sisl.geom.graphene().lattice
<sisl.Lattice a=2.4595, b=2.4595, c=14.2, α=90.0, β=90.0, γ=60.0, bc=[P, P, P], nsc=[3 3 1]>

Could one enable simple string specifications, like boundary_condition="ppu" etc?

pfebrer · 2023-10-23T10:16:58Z

I like Thomas' third point :) But I don't know if it deviates too much from the approach you were taking.

zerothi · 2023-10-23T10:22:39Z

I think this looks good. Just some simple observations:

1. `boundary_condition=True` is accepted, but is this intended?

>>> sisl.Lattice(1, boundary_condition=True)
<sisl.Lattice a=1.0, b=1.0, c=1.0, α=90.0, β=90.0, γ=90.0, bc=[U, U, U], nsc=[1 1 1]>

Thanks for discovering this, I'll amend some checks.

2. Would it make sense to set the z-direction to be nonperiodic in the 2D geometries?

>>> sisl.geom.graphene().lattice
<sisl.Lattice a=2.4595, b=2.4595, c=14.2, α=90.0, β=90.0, γ=60.0, bc=[P, P, P], nsc=[3 3 1]>

Yes, this should be incorporated into the geom creation routines. Agreed!
I'll amend.

3. Could one enable simple string specifications, like `boundary_condition="ppu"` etc?

I don't like that... With @pfebrer addition one can do strings, but they should be part of a list. right now (not committed) this would work:

lattice.set_boundary_condition(a="per", b="un") # uses `BC.name.startswith(key.upper())`
# your proposal would be:
boundary_condition=list("ppu")

Basically:

a single string would be broadcasted to all directions
a list of strings would be broadcasted to each direction
a list of list of strings would be individual boundaries
Hope this would satisfy the usage.

pfebrer · 2023-10-23T10:30:12Z

You made the same point to me when axes in sisl.viz only accepted a list, pointing that it would be better to pass "xy" instead of list("xy") or ["x", "y"]. I think that it has showed to be the most convenient way and probably it would also be very convenient in this case.

zerothi · 2023-10-23T10:33:38Z

You made the same point to me when axes in sisl.viz only accepted a list, pointing that it would be better to pass "xy" instead of list("xy") or ["x", "y"]. I think that it has showed to be the most convenient way and probably it would also be very convenient in this case.

Here it is a bit more generic... In viz there are only ever 3 axes, "xyz", and "abc", while for boundary conditions we don't really know if there later will be a new condition called "Poisson_*", in which case "P" is a bad specifier.
The point is, boundary condition is not necessarily a static entity, in which case single letter specification is a bad notation. I agree with the current names they are unique, but, I am more concerned that this will be the preferred way of assigning BC, but that would limit future implementations.

zerothi · 2023-10-23T10:38:29Z

Actually, startswith is equally bad... But it would encourage more than 1 letter words. I'll add so 1 letter words are not accepted.

pfebrer · 2023-10-23T10:38:43Z

There are also more complex axes like a specific direction [1, 1, 0]. They are simply not allowed in the string syntax. I think it could be possible here as well that the string syntax is a shorthand that only supports the most common cases and if you have something more complex then you go with the "normal" syntax.

pfebrer · 2023-10-23T10:39:40Z

Actually, startswith is equally bad... But it would encourage more than 1 letter words. I'll add so 1 letter words are not accepted.

Yeah I would just specify explicitly a shorthand string for each case, as you do with spins (I think).

zerothi · 2023-10-23T10:40:13Z

There are also more complex axes like a specific direction [1, 1, 0]. They are simply not allowed in the string syntax. I think it could be possible here as well that the string syntax is a shorthand that only supports the most common cases and if you have something more complex then you go with the "normal" syntax.

What about: "per,un,per"? Then a comma/colon/whatever separator would allow this more easily?

pfebrer · 2023-10-23T10:43:03Z

I think a list is better than commas in a string, so as a first thought I wouldn't say it would help with usage :)

zerothi · 2023-10-23T10:57:01Z

There are also more complex axes like a specific direction [1, 1, 0]. They are simply not allowed in the string syntax. I think it could be possible here as well that the string syntax is a shorthand that only supports the most common cases and if you have something more complex then you go with the "normal" syntax.

But what is the most common cases? The only non-common thing is "UNKNOWN"...

I think a list is better than commas in a string, so as a first thought I wouldn't say it would help with usage :)

Ok, I think I can accept that updno always refer to the currently known boundary conditions. I'll amend.

pfebrer · 2023-10-23T11:06:12Z

But what is the most common cases? The only non-common thing is "UNKNOWN"...

I think more than 90 % of the time people will use periodic or open. But maybe I'm wrong.

pfebrer · 2023-10-23T11:07:36Z

I didn't realise there was unknown, how should it be treated? Should it raise errors when processing is attempted?

zerothi · 2023-10-23T12:02:43Z

I didn't realise there was unknown, how should it be treated? Should it raise errors when processing is attempted?

No, it is a placeholder for something the user doesn't know. It could be unspecified, or simply "unknown".

But what is the most common cases? The only non-common thing is "UNKNOWN"...

I think more than 90 % of the time people will use periodic or open. But maybe I'm wrong.

Hmm, I think the others could be equally nice. :)

sisl.geom geometries intrinsically sets the PBC single string for short setting them is now doable Signed-off-by: Nick Papior <[email protected]>

zerothi · 2023-10-23T13:16:46Z

I have amended your requests, could you have a look.

codecov · 2023-10-23T13:16:51Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (7145e58) 87.68% compared to head (55cb1bd) 87.73%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #629      +/-   ##
==========================================
+ Coverage   87.68%   87.73%   +0.05%     
==========================================
  Files         361      362       +1     
  Lines       48080    48302     +222     
==========================================
+ Hits        42158    42378     +220     
- Misses       5922     5924       +2

Files	Coverage Δ
src/sisl/geom/_common.py	`100.00% <100.00%> (ø)`
src/sisl/io/tests/test_xyz.py	`100.00% <100.00%> (ø)`
src/sisl/physics/densitymatrix.py	`90.87% <100.00%> (+0.01%)`	⬆️
src/sisl/physics/electron.py	`83.52% <100.00%> (+0.02%)`	⬆️
src/sisl/tests/test_grid.py	`100.00% <100.00%> (ø)`
src/sisl/tests/test_lattice.py	`99.27% <100.00%> (+0.07%)`	⬆️
src/sisl/io/xyz.py	`95.87% <95.23%> (+0.08%)`	⬆️
src/sisl/grid.py	`86.96% <92.85%> (-0.28%)`	⬇️
src/sisl/lattice.py	`93.09% <94.16%> (+0.05%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pfebrer · 2023-10-23T13:20:28Z

But for example, if the user asks to compute neighbours, what should happen?

zerothi · 2023-10-23T13:22:15Z

But for example, if the user asks to compute neighbours, what should happen?

I agree, that is one of the points that needs resolution, but perhaps this is more appropriately solved with #553?

The whole point is that nsc should control the search space...
While one could argue that nsc is not something the user should control, in has proven to be very educational. Users are required to think about the concept of ranges, and can physically see them through nsc.

pfebrer · 2023-10-23T13:26:11Z

The whole point is that nsc should control the search space...

I don't agree with this. Boundary conditions should control the search space. Otherwise it's very confusing because the boundary conditions can be meaningless... (e.g. open boundary conditions with nsc=[3, 3, 3]).

pfebrer · 2023-10-23T13:27:47Z

Or periodic boundary conditions with nsc=[3, 3, 3] while in reality there are interactions with second neighbour cells. If you follow nsc you are not using periodic boundary conditions then.

zerothi · 2023-10-23T13:27:54Z

The whole point is that nsc should control the search space...

I don't agree with this. Boundary conditions should control the search space. Otherwise it's very confusing because the boundary conditions can be meaningless... (e.g. open boundary conditions with nsc=[3, 3, 3]).

One could do a transiesta calculation with nsc=3 but you would have open boundary conditions.
It ain't pretty, it ain't right. But what would you do? When specifying anything but periodic it also sets nsc to 1?

pfebrer · 2023-10-23T13:29:32Z

When specifying anything but periodic it also sets nsc to 1?

If it is not periodic, then nsc doesn't make any sense, no? So there simply shouldn't be a nsc attribute in that case.

zerothi · 2023-10-23T13:31:49Z

Or periodic boundary conditions with nsc=[3, 3, 3] while in reality there are interactions with second neighbour cells. If you follow nsc you are not using periodic boundary conditions then.

no, but that could be used to limit the interaction range (intentionally). That is currently a flexibility in sisl where one can create a graphene flake, but set nsc = 1 to create a GNR.
I agree it is confusing, but...

I am very much pro making this intuitive, and sustainable, but the way things are moving, #553 in particular, makes me wonder how we actually should implement these constraints, because users could do:

H.geometry.lattice.set_boundary_condition(...)

and the Hamiltonian wouldn't know anything about it...

When specifying anything but periodic it also sets nsc to 1?

If it is not periodic, then nsc doesn't make any sense, no? So there simply shouldn't be a nsc attribute in that case.

I agree. But... The way the objects are interacting is probably not the best, I just can't see how we should move things around. See e.g. the above example which disrupts the full H

pfebrer · 2023-10-23T13:42:06Z

no, but that could be used to limit the interaction range (intentionally). That is currently a flexibility in sisl where one can create a graphene flake, but set nsc = 1 to create a GNR.

I agree the flexibility is nice. But it should be an opt-in thing. I remember being very confused at the beggining when I created a geometry and close didn't find the periodic neighbours, and it was not very nice to have to set nsc manually because I didn't know if it was 3, 5, 7... So yeah in the routines that use an auxiliary cell you could have some option to limit interactions, but by default the boundary conditions should be sufficient to specify what you want to happen.

and the Hamiltonian wouldn't know anything about it...

I think you shouldn't be able to modify a Hamiltonian's geometry. So once it is associated, a frozen copy should be made. You could change the boundaries from periodic to open and the Hamiltonian should then discard all supercell interactions. Or you could translate atoms from one cell to another, and the Hamiltonian should adapt. As in translate2uc. But I don't know if there are any other operations that could make sense.

pfebrer · 2023-10-23T13:44:19Z

So in practical terms maybe the method that sets boundary conditions should create a new Lattice object? Just as the methods that return modified geometries do?

zerothi · 2023-10-23T14:14:45Z

no, but that could be used to limit the interaction range (intentionally). That is currently a flexibility in sisl where one can create a graphene flake, but set nsc = 1 to create a GNR.

I agree the flexibility is nice. But it should be an opt-in thing. I remember being very confused at the beggining when I created a geometry and close didn't find the periodic neighbours, and it was not very nice to have to set nsc manually because I didn't know if it was 3, 5, 7... So yeah in the routines that use an auxiliary cell you could have some option to limit interactions, but by default the boundary conditions should be sufficient to specify what you want to happen.

That's what optimize_nsc is for. ;)
But I see your point. How should we go about this. nsc is an advanced thing, but also ideal for users to know about.

and the Hamiltonian wouldn't know anything about it...

I think you shouldn't be able to modify a Hamiltonian's geometry. So once it is associated, a frozen copy should be made. You could change the boundaries from periodic to open and the Hamiltonian should then discard all supercell interactions. Or you could translate atoms from one cell to another, and the Hamiltonian should adapt. As in translate2uc. But I don't know if there are any other operations that could make sense.

I agree, but I don't know how to do this simply. Many of the routines does things in-place, so essentially all changing things should return a new object.

tfrederiksen · 2023-10-23T20:46:49Z

I played a bit around with this and it looks good to me.

zerothi · 2023-10-24T10:22:33Z

I played a bit around with this and it looks good to me.

@tfrederiksen what do you think about the names? Is it too long? Should we do with bc, or bound_cond, comments?

tfrederiksen · 2023-10-24T10:40:57Z

I don't like bound_cond too much (neither a short abbreviation nor completely explicit). bc could be OK and suitable like with pbc. On the other hand, I am more in favour of the explicit form boundary_condition which makes the code easier to read. It would be aligned with the property .lattice and the function set_boundary_condition(...), etc. bc has also the drawback that it resembles two of the directional arguments like in latt.set_boundary_condition(b=per, c='un), but this may be a minor point. Could one accept both, something like bc = boundary_condition for the lazy user?

zerothi · 2023-10-24T10:42:49Z

I don't like bound_cond too much (neither a short abbreviation nor completely explicit). bc could be OK and suitable like with pbc. On the other hand, I am more in favour of the explicit form boundary_condition which makes the code easier to read. It would be aligned with the property .lattice and the function set_boundary_condition(...), etc. bc has also the drawback that it resembles two of the directional arguments like in latt.set_boundary_condition(b=per, c='un), but this may be a minor point. Could one accept both, something like bc = boundary_condition for the lazy user?

Thanks, lets keep it for now! :)

This should make users aware of some inconsistencies in the functionality of the objects. Signed-off-by: Nick Papior <[email protected]>

zerothi · 2023-10-24T10:53:17Z

@pfebrer the latest commit adds an informational warning that is shown when a boundary condition is changed from PERIODIC to non-periodic and nsc>1. It will only be shown if the bc is changed.

This is then a place-holder to inform users of inconsistencies, but users are expected to correct it them-selves.
I would be careful about changing nsc since it can be attached to a Hamiltonion. So this should be a warning for now.

pfebrer · 2023-10-24T14:18:49Z

Yes I also agree it should be either boundary_condition or bc. I prefer the second, but both can be fine.

@pfebrer the latest commit adds an informational warning that is shown when a boundary condition is changed from PERIODIC to non-periodic and nsc>1. It will only be shown if the bc is changed.

Ok, let's see how you make nsc play with the neighbour finder, because there currently I just don't care about it and calculate the nsc needed for periodic boundary conditions 😅

zerothi · 2023-10-24T19:16:21Z

Yes I also agree it should be either boundary_condition or bc. I prefer the second, but both can be fine.

@pfebrer the latest commit adds an informational warning that is shown when a boundary condition is changed from PERIODIC to non-periodic and nsc>1. It will only be shown if the bc is changed.

Ok, let's see how you make nsc play with the neighbour finder, because there currently I just don't care about it and calculate the nsc needed for periodic boundary conditions 😅

Hehe, ok, yes, lets play with that as well. The main constraint is that the offsets in the sparse matrix is a 1-1 correspondence with the offsets.

zerothi · 2023-10-24T19:27:51Z

I'll keep the current state and merge, thanks a lot for your feedback!

github-advanced-security bot found potential problems Oct 20, 2023

View reviewed changes

zerothi mentioned this pull request Oct 23, 2023

Included missing notebook in the viz docs #630

Merged

amended requests from pfebrer and tfrederiksen

8979d25

sisl.geom geometries intrinsically sets the PBC single string for short setting them is now doable Signed-off-by: Nick Papior <[email protected]>

Added info message when non-periodic and nsc>1

55cb1bd

This should make users aware of some inconsistencies in the functionality of the objects. Signed-off-by: Nick Papior <[email protected]>

zerothi merged commit fb8a39f into main Oct 24, 2023

zerothi deleted the lattice-bc branch October 24, 2023 19:28

Lattice is now owning the boundary conditions #629

Lattice is now owning the boundary conditions #629

Conversation

zerothi commented Oct 20, 2023 • edited Loading

zerothi commented Oct 20, 2023

pfebrer commented Oct 20, 2023 • edited Loading

zerothi commented Oct 21, 2023

zerothi commented Oct 22, 2023

tfrederiksen commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

pfebrer commented Oct 23, 2023 • edited Loading

zerothi commented Oct 23, 2023

zerothi commented Oct 23, 2023

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

pfebrer commented Oct 23, 2023

pfebrer commented Oct 23, 2023

zerothi commented Oct 23, 2023

tfrederiksen commented Oct 23, 2023

zerothi commented Oct 24, 2023

tfrederiksen commented Oct 24, 2023

zerothi commented Oct 24, 2023

zerothi commented Oct 24, 2023

pfebrer commented Oct 24, 2023

zerothi commented Oct 24, 2023

zerothi commented Oct 24, 2023

zerothi commented Oct 20, 2023 •

edited

Loading

pfebrer commented Oct 20, 2023 •

edited

Loading

pfebrer commented Oct 23, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading