KpointsData mesh doesn't work in BandsData

[greschd]

The following doesn't work:

kmesh = DataFactory('array.kpoints')()
kmesh.set_kpoints_mesh([4, 4, 4])
bands = DataFactory('array.bands')()
bands.set_kpointsobj(kmesh)

This is because the KpointsData behaves differently depending on how it was set (with set_kpoints, set_kpoints_mesh, or set_kpoints_path). I think this can be improved by doing one of the following:

• keeping the way KpointsData is constructed, but providing some common interface (e.g. extend get_kpoints to always return the explicit k-points).
• splitting KpointsData into a base class containing the common interface, and then creating subclasses for explicit k-points, k-point meshes and k-points from paths.

Any thoughts on this?

[giovannipizzi]

Thanks. Indeed, the current way the kpoints class behaves is a bit weird, as it raises exceptions depending on its content. However, some code currently depends on this behavior... I would be ok to change it if we find a proper interface. I a bit reluctant to make subclasses because this would change all the DB contents - probably there is a good way to return the correct value (e.g. return None rather than raising if the mesh was not set; and have a method that tells what is stored - what do you think?)

In any case, for this issue:

• I'm not sure there is a set_kpointskmeshobj method in the codebase, am I mistaken?
• is the method kmesh.get_kpoints_mesh(print_list=True) enough to achieve your task?

[greschd]

I have a workaround for the issue, which is just to use get_kpoints_mesh(print_list=True) to create a new instance with explicit k-points.

Maybe a lot of the problems would be solved by just returning get_kpoints_mesh(print_list=True) when get_kpoints() is called on a 'mesh' object. However I don't know if some code might actually be relying on this not working. A more 'drastic' approach would be to create new Data classes for k-points, and just provide a way to get them from the current interface (and then slowly deprecating the old classes). This would ensure that current code is not broken, but of course there would be a bit of a hassle in working with code which contains both 'new' and 'old' classes. I guess it's safe to assume that a lot of code interfaces to KpointsData, so we should consider changes carefully.

[nmounet]

I confirm that a number of workflows, scripts, etc. are heavily relying on the failing of "get_kpoints" when only a mesh was defined. I'm quite in favour of the 'drastic' approach (because back-compatibility is my leitmotiv...).

[giovannipizzi]

I am, instead, in favour of improving the python API of kpoints (as it is it's a mess).

I think:

• get_kpoints returns the explicit list of points anyway (maybe we can find a different name, so we can deprecate get_kpoints: e.g., get_kpoints_list())
• get_explicit_kpoints returns what kpoints has now, and None (instead of an exception) if it's a mesh
• get_kpoints_mesh returns None rather than raising an exception
• we add a method (e.g. is_mesh, or maybe better is_explicit since we also have the 'Gamma-only' case) that tells you what the class is.

Fixing the workflows that depend on this (weird) behaviour should take a couple of hours maximum, and requires to simplify the code mainly. If one really needs to work on both cases, one can write a wrapper function that does what is needed, understanding on which version it's working (e.g. checking hasattr(kpoints, 'is_mesh') or something like this.

What do you think?

[greschd]

Since the different 'kinds' of KpointsData (mesh, path, list) are constructed differently, and have a partially different subset of functionality, it makes more sense to me to make them different subclasses of a common base.

In general, returning None instead of raising an exception doesn't solve the problem. It just means the user code will have to handle the None case specially -- which is IMO not better than the user code having to handle the exception.

Another advantage of having different subclasses is that it decouples the base kpoints API from the DB representation. For example, ArrayData might not be the easiest way to represent the k-point path.

[sphuber]

Postponed to v2.0.0 because the changes would be too big so short before v1.0.0 release