nsc should not belong to Lattice

[zerothi]

          It is weird because `nsc` is not really a property of the lattice, no? It only makes sense in a combination of a lattice, some sites, and something that defines the range of the sites. Perhaps the lattice information should be separated from the auxiliary cell information, which could be an attribute -`aux_cell`- of `Geometry` or whatever other class where it makes sense.

So lattice would hold cell and pbc, and aux_cell would hold nsc.

Originally posted by @pfebrer in #550 (comment)

[zerothi]

I agree, it would be nice to put the supercells into the placeholder that really uses them.

In this case it would be the Geometry class.

Considerations for alternate names,

• neighbour_cells
• supercells
• aux_cells
• others?
  I kind of have a hard time going around the plural s, neighbour_cell only signifies one direction in this case. @tfrederiksen please chip in here as well.

[pfebrer]

I think of it as just a bigger cell, so I don't have a problem with removing the plural 😅

[tfrederiksen]

What about supercell_shape? Or variants hereof: supercell_size, supercell_range...

[zerothi]

What about supercell_shape? Or variants hereof: supercell_size, supercell_range...

Kind of like supercell_size/range/extent, has meaning and clarity.... But still a bit long... Hmm...

[zerothi]

So should this be a class in it-self, probably that would be the best thing. Since Geometry, Grid should use this feature.

[tfrederiksen]

Could Supercell be a child of Lattice with the extra info about a supercell_size etc?

[zerothi]

As @pfebrer pointed out this would still be a bit ambiguous. The supercell is intrinsically connected to some ranged property (orbital in this case) + the lattice vectors. So it is not only due to the Lattice, it is some combination of the Atom + coordinates + Lattice that results in the supercell. At least conceptually. Users may then overwrite the intrinsic supercells.

The idea that Lattice has pbc would be useful, but it might also be a bit problematic downstream if pbc[i] == True but nsc[i] == 1, what to do then?

[pfebrer]

The part I don't understand about SIESTA's way of handling supercells is why aren't atoms moved into the unit cell for computing neighbors. Because then computing the number of supercells needed would be much simpler, no? E.g. you would be able to tell if you need nsc > 3 just from the maxR of your Atoms object and if pbc=True you could assume nsc >= 3. So computing nsc would then not depend on the coordinates and would be just a function of Lattice and Atoms.

[pfebrer]

The idea that Lattice has pbc would be useful, but it might also be a bit problematic downstream if pbc[i] == True but nsc[i] == 1, what to do then?

In principle a structure can be periodic even if orbitals don't overlap, no? So pbc=True, nsc=1 is not contradictory by itself. Maybe not common, but possible.

[pfebrer]

I guess pbc is something that the user can decide and set to whatever they want, but nsc is not something that the user should be able to set arbitrarily.

[zerothi]

The part I don't understand about SIESTA's way of handling supercells is why aren't atoms moved into the unit cell for computing neighbors. Because then computing the number of supercells needed would be much simpler, no? E.g. you would be able to tell if you need nsc > 3 just from the maxR of your Atoms object and if pbc=True you could assume nsc >= 3. So computing nsc would then not depend on the coordinates and would be just a function of Lattice and Atoms.

That's not entirely true. It might be for simple cells, but for some cells with angles very large/small the calculation of supercells is not merely a case of orbital ranges. Secondly, in some cases you can shrink the required supercell depending on atomic positions since their overlaps do depend on positions.

In principle a structure can be periodic even if orbitals don't overlap, no? So pbc=True, nsc=1 is not contradictory by itself. Maybe not common, but possible.

True, ok, but what about pbc=False, nsc=3 that would be weird no... But I agree, pbc is a lattice setting (Poisson problem), nsc is an orbital range setting (matrix hoppings).

I guess pbc is something that the user can decide and set to whatever they want, but nsc is not something that the user should be able to set arbitrarily.

Agreed, but manually overriding nsc has turned out to be extremely handy when dealing with truncations of connections. :)
So it would be ideal to keep its manual setting in some form.

[zerothi]

You can see in the Siesta code that the simple estimation of nsc is in fact done only via max-orbital ranges and the lattice parameters. But it is often different than the simple one.

[zerothi]

Comment for remembering: the object holding nsc should also reference the Lattice since a table of offsets are created based on nsc and the internal isc_off table.

[zerothi]

Would it make sense to make nsc part of geometry, that is the only class that has all the information.

Grid does not use it unless an associated geometry is hosted, and lattice also doesn't need it.

[pfebrer]

And who will store the supercell offsets?

[zerothi]

Agreed, it just becomes clunky to have a class which collects the geometry and the lattice, but then it gets stored in the geometry.

Wouldn't it be better if the geometry contained the integer offsets, and when requesting real offsets, they are calculated (very simple MM).

[tfrederiksen]

The part I don't understand about SIESTA's way of handling supercells is why aren't atoms moved into the unit cell for computing neighbors. Because then computing the number of supercells needed would be much simpler, no? E.g. you would be able to tell if you need nsc > 3 just from the maxR of your Atoms object and if pbc=True you could assume nsc >= 3. So computing nsc would then not depend on the coordinates and would be just a function of Lattice and Atoms.

I also start to wonder about this. For instance this behaviour seems a bit odd to me:

>>> g = sisl.geom.graphene()
>>> g.xyz[0] += 10 * g.cell[0] # translate atom by multiple of lattice vector
>>> for ia in g:
...    print(ia, g.close(ia, R=1.5)) # no neighbours found because nsc = [3, 3, 1]
... 
0 [0]
1 [1]
>>> g.optimize_nsc(R=20)
array([35, 19,  3], dtype=int32)
>>> for ia in g:
...    print(ia, g.close(ia, R=1.5))
... 
0 [   0 1947 2013 2015]
1 [   1 1976 1978 2044]

In the first close call no first neighbours are found (although due to periodicity there are physical neighbours). In the second call, they are found but nsc is ridiculously large for the problem.

Thus, it seems that having atoms outside the primitive cell is prone to complications or errors.

[zerothi]

Thats a current known bug in sisl... :(
My plan was to revisit this when #393 gets in (I am just too busy at the time being).