zerothi / sisl

Electronic structure Python package for post analysis and large scale tight-binding DFT/NEGF calculations
https://zerothi.github.io/sisl
Mozilla Public License 2.0
201 stars 60 forks source link

`only` or `what`? #541

Closed tfrederiksen closed 1 year ago

tfrederiksen commented 1 year ago

539 made me notice that the keyword in Geometry.swapaxes is what, while in Geometry.rotate it is only. Would it make sense to unify? I like what better.

zerothi commented 1 year ago

I agree. Unification! Grep for what, only, which, how, others we could have used, to ensure we get all.

tfrederiksen commented 1 year ago

Good. By the way, I think what='xyz' could be the better default in rotate. At least I have never used rotation of axes and thus need to set this keyword every time.

zerothi commented 1 year ago
sisl % grep -E "[^A-Za-z0-9_](only|which|what|how)[^A-Za-z0-9_]" **/*.py | grep " def "                                                                                                          main
geometry.py:    def rotate(self, angle, v, origin=None, atoms: Optional[AtomsArg]=None, only='abc+xyz', rad=False) -> Geometry:
geometry.py:    def center(self, atoms: Optional[AtomsArg]=None, what="xyz") -> ndarray:
geometry.py:    def scale(self, scale, what="abc", scale_atoms=True) -> Geometry:
io/tbtrans/tbt.py:    def orbital_transmission(self, E, elec=0, kavg=True, isc=None, only="all", orbitals=None) -> csr_matrix:
io/tbtrans/tbt.py:    def orbital_current(self, elec=0, elec_other=1, kavg=True, isc=None, only="all", orbitals=None) -> csr_matrix:
io/tbtrans/tbt.py:    def bond_transmission(self, E, elec=0, kavg=True, isc=None, only="all", orbitals=None, uc=False) -> csr_matrix:
io/tbtrans/tbt.py:    def bond_current(self, elec=0, elec_other=1, kavg=True, isc=None, only="all", orbitals=None, uc=False) -> csr_matrix:
io/tbtrans/tbt.py:    def vector_transmission(self, E, elec=0, kavg=True, isc=None, only="all", orbitals=None) -> ndarray:
io/tbtrans/tbt.py:    def vector_current(self, elec=0, elec_other=1, kavg=True, isc=None, only="all", orbitals=None) -> ndarray:
sparse_geometry.py:    def rij(self, what='orbital', dtype=np.float64):
sparse_geometry.py:    def Rij(self, what='orbital', dtype=np.float64):
supercell.py:    def rotate(self, angle, v, only='abc', rad=False):
supercell.py:    def scale(self, scale, what="abc"):
viz/input_fields/orbital.py:    def _split_query(self, query, on, only=None, exclude=None, query_gen=None, ignore_constraints=False, **kwargs):
viz/input_fields/orbital.py:    def _generate_queries(self, on, only=None, exclude=None, query_gen=None, **kwargs):
viz/plots/fatbands.py:    def split_groups(self, on="species", only=None, exclude=None, clean=True, colors=(), **kwargs):
viz/plots/pdos.py:    def split_requests(self, *i_or_names, on="species", only=None, exclude=None, remove=True, clean=False, ignore_constraints=False, **kwargs):
viz/plots/pdos.py:    def split_DOS(self, on="species", only=None, exclude=None, clean=True, **kwargs):
zerothi commented 1 year ago

@tfrederiksen do you have a suggestion for tbtrans, should we change those?

tfrederiksen commented 1 year ago

I think what would work in all cases above.

tfrederiksen commented 1 year ago

Well, maybe the meaning is somewhat different in viz with only and exclude?

zerothi commented 1 year ago

Well, maybe the meaning is somewhat different in viz with only and exclude?

true, lets keep them.

zerothi commented 1 year ago

Good. By the way, I think what='xyz' could be the better default in rotate. At least I have never used rotation of axes and thus need to set this keyword every time.

actually, changing the default to "xyz" breaks the nanoribbon and bilayer creation routines since they rely on also rotating the lattice vectors.
I can see that there may be multiple use-cases.

  1. When rotating geometries for attaching them, "xyz+abc" is preferred,
  2. When rotating sub-segments "xyz" is preferred.

So which has precedence? I think you are correct that 2. is mostly used, so lets stick with that, this is just to at least be aware of the implications.

tfrederiksen commented 1 year ago

OK I see. Would it make sense that what="xyx" is the default if atoms is specified, otherwise "xyz+abc"?

zerothi commented 1 year ago

I think that complicates things... I have this somewhere, but when you don't use the routines often, then it makes more sense to have a stable default... well, I don't know.

Is my above understanding even correct, are there other cases I haven't thought of?

zerothi commented 1 year ago

OK I see. Would it make sense that what="xyx" is the default if atoms is specified, otherwise "xyz+abc"?

Implementing this and fixing stuff all-around made me convinced that it is ok to have two different defaults, I have done as you suggested ;)