Add the ElectronicBandsData data plugin

[sphuber]

Fixes #5032

This data plugin is a subclass of the BandsData plugin. It adds
support to define a fermi energy, which is set as an attribute. This is
a typical use case for electronic band structures, which so far were
using the BandsData type, but the latter was also being used for other
band structures, such as phonons, which do not share this property.

By making it a subclass, existing processes that accept BandsData
instances will automatically also accept ElectronicBandsData instances
and queries for BandsData will also match the subclasses, unless the
user turns of subclassing explicitly.

[sphuber]

Probably need to add a fermi_energy_unit property as well, but first wanted to gauge if this solution might cover all use cases that we know off.

[codecov]

Codecov Report

Merging #5033 (e473aac) into develop (a468519) will increase coverage by 0.03%.
The diff coverage is 96.56%.

@@             Coverage Diff             @@
##           develop    #5033      +/-   ##
===========================================
+ Coverage    80.23%   80.25%   +0.03%     
===========================================
  Files          515      517       +2     
  Lines        36757    36784      +27     
===========================================
+ Hits         29489    29519      +30     
+ Misses        7268     7265       -3     

Flag	Coverage Δ
django	74.75% <96.56%> (+0.03%)	⬆️
sqlalchemy	73.65% <96.56%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_data/cmd_show.py	16.67% <0.00%> (ø)
aiida/orm/nodes/data/array/bands/__init__.py	100.00% <100.00%> (ø)
aiida/orm/nodes/data/array/bands/bands.py	76.55% <100.00%> (ø)
aiida/orm/nodes/data/array/bands/electronic.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a468519...e473aac. Read the comment docs.

[mbercx]

Thanks @sphuber! Just left a few questions/comments, and still have a more general one that's a bit of semantic nit-picking. The terms Fermi energy and Fermi level are often used interchangeably, but I think it's more precise to use Fermi energy for "the energy difference between the highest and lowest occupied single-particle states in a quantum system of non-interacting fermions at absolute zero temperature":

https://en.wikipedia.org/wiki/Fermi_energy

Whereas the Fermi level is the one we are adding to the BandsData here, I think:

https://en.wikipedia.org/wiki/Fermi_level

[mbercx]

I must admit I do not like this constructor using **kwargs from a UX point of view. ^^ Basically it's impossible to tell for a user that checks the docstring how to use it. What's the advantage of this, and does it outweigh the benefit of the user simply seeing the input arguments and having them documented in the docstring?

[sphuber]

I have changed the interface. However, it does bring other potential use problems with it. See the commit message for an explanation. Using the / marker to make them positional only "solves" the problem somewhat.

[sphuber]

Hmmm, the / positional argument only marker is only supported for Python 3.8 and up. Not sure if there is a backport solution for this, but I doubt it since it concerns the basic syntax. @chrisjsewell do you happen to know?

[sphuber]

Thanks for the review @mbercx . Fine for me to change to fermi_level. Maybe it would be good to raise this point during a CWF meeting and see that others agree. Would also be a good moment to see if people think we should add an attribute for the energy unit or whether we enforce eV as has been the norm for data types in aiida-core so far. I think being explicit and storing the unit is the better option even though that means we'd be breaking with current "standard" practice.

[giovannipizzi]

If the units in BandsData are always eV (I think this should be the case now that we are defining a Electronic type), I would vote to also force the fermi_level to be in eV and document it. I think it just makes our life too complex if we need to then define a new layer to convert units between various ElectronicBandsData. If we really want to have multiple units, I still think the units of the Fermi level should be the same as for the bands, so we would define only one unit for bands and the Fermi level.

Also, if we want to go down this route of defining an ElectronicBandsData type, we should move in the new type all functionality that is specific to electronic bands (two bands channels = spin up/down, the find_bandgap function, ...)?
I'm still wondering if this is really useful, in the sense that for me it's a bit different than the case of Kpoints, where we really were storing two different types of kpoints (grid vs. explicit points), and some methods just except if the other type has been set. Here there's just the definition of a "zero", and one could give a more general name (energy_zero for instance). So I think that having a subclass is less critical. But anyway, I'm not against it if we properly move electron-specific methods to this subclass and the users won't notice when the plugin change type.

[mbercx]

Also, if we want to go down this route of defining an ElectronicBandsData type, we should move in the new type all functionality that is specific to electronic bands (two bands channels = spin up/down, the find_bandgap function, ...)?

I'm definitely in favor of implementing the ElectronicBandsData type, and adding the methods you mention. The find_bandgap method would really be useful, I think. How would I now obtain a band gap from a BandsData node? This should be very straightforward for users.

[giovannipizzi]

How would I now obtain a band gap from a BandsData node?

Ok, my fault - I was under the false impression that there was a band.find_bandgap() method, instead there is only a function. I would suggesting to move this to aiida.tools.data. To make it easy, I think we should add also the datanode.tools.* extensions similar to what I think we have for calcjobs already, but I don't think we have yet for data nodes? So one can just add things to aiida.tools rather than to the class itself, that probably should just focus son how to store the data internally, but still you will be able to do bandnode.tools.find_bandbap rather than

from aiida.tools.data.array.bands import find_bandgap
find_bandgap(bandnode)

(with the additional benefit that one can do node.tools.<TAB> to discover which tools exist for that node type.

I can't find the issue on data.tools, maybe @sphuber remembers? I think we discussed on the train back from the 2019 coding week.

EDIT: this is the corresponding .tools for calcjobs:

aiida-core/aiida/orm/nodes/process/calculation/calcjob.py

Line 54 in e274ef2

def tools(self) -> 'CalculationTools':

[sphuber]

I can't find the issue on data.tools, maybe @sphuber remembers? I think we discussed on the train back from the 2019 coding week.

The reason that there is no data.tools analogue to the calculation.tools is because they do not have the same problem. The reason that we implemented the calculation tools concept was because the process node classes in AiiDA are not subclassable. This means that people cannot add their own custom functionality such as utility methods that operate on the various process nodes. The same does not go for Data nodes. These are subclassable and so any utility functions can be added directly on those classes. If the ElectronicBandsData needs a get_band_gap method, then we should just implement on that class. I don't see the added value of implementing this elsewhere through some complicated entry point system.

[mbercx]

Regarding placing these in e.g. BandsData.tools.<method_name>: Although I do understand that adding these here will make them easier to find considering the vast number of methods available for all node classes, we do need to make sure that this is documented very well, and also introduced at the tutorial.

However, I would be more in favor of not having this kind of workaround, which again requires us to teach the users something to just find the utility methods they need. Instead, it would be better if we could hide methods that users typically do not need, so they can more easily find the methods that we would place in .tools. I think I already discussed this with @chrisjsewell at some point, and he mentioned a way to do this elegantly which I now forgot. (this was in the context of #4975)

[giovannipizzi]

OK to keep it in the main class, but indeed for a user it's overwhelming to find all node methods (that they might not need) + the methods acting on the nodes. Also this makes data classes quite complex, again with a design principle that I would like to push forward that we keep data classes as stable as possible over time, just focusing on storing data in the DB in a consistent format, and move code that processes it outside.

As a historical note we had tried in AiiDA 0.x to hide methods (prefixing with underscore) that might not be needed by a end user (e.g. set_attribute), but then we agreed that this was not the right approach, as it was actually hiding methods that, from a python point of view, were not really private, as e.g. a plugin or some other AiiDA-core class might want to call them to set an attribute.
If there is some other way to do the "right" thing I'm all ears (maybe it's overriding the __dir__? But I don't know if I would do it, the user would be confused of not finding a method in dir but still be able to use it. Namespacing feels better to me.

But anyway this is also a matter of taste, and it's not crucial for this issue.

[mbercx]

Fine for me to change to fermi_level. Maybe it would be good to raise this point during a CWF meeting and see that others agree.

Just a note here: This was discussed during a recent CWF meeting, and I think it was agreed that Fermi level is a slightly more correct terminology. However, I did not get the impression that everyone was as passionate about this sematic nit-picking as I. 😅

[sphuber]

Will close this PR for now since it has been inactive for a long time and I don't anticipate having time soon to address the changes. Will reopen when there is a new design incorporating the suggested changes.