3. How properties should be stored inside the StructureData Node about aiida-atomistic HOT 7 OPEN

Up to now, in the current implementation of the atomistic StructureData (main branch), all the properties are under the properties attribute directly:

unit_cell = [[3.5, 0.0, 0.0], [0.0, 3.5, 0.0], [0.0, 0.0, 3.5]]
atomic_positions = [[0.0, 0.0, 0.0],[1.5, 1.5, 1.5]]
symbols = ["Li"]*2
mass = [6.941,6.941]
charge = [1,0]

properties = {
    "cell":{"value":unit_cell},
    "pbc":{"value":[True,True,True]},
    "positions":{"value":atomic_positions,},
    "symbols":{"value":symbols},
    "mass":{"value":mass,},
    "charge":{"value":charge}
    }

structure = StructureData(
        properties=properties
        )

we can access the properties in this way:

structure.properties.cell 
structure.properties.symbols
...

Each property has an attribute called domain, which can be global (pbc, cell) intra-site (mass, charge,positions,symbols) or inter-site (Hubbard V, not yet implemented).

Another possibility is to have this domains (maybe with different names) as attributes of the properties, and then under these three have the related properties (structure.properties.global.cell ... ):

unit_cell = [[3.5, 0.0, 0.0], [0.0, 3.5, 0.0], [0.0, 0.0, 3.5]]
atomic_positions = [[0.0, 0.0, 0.0],[1.5, 1.5, 1.5]]
symbols = ["Li"]*2
mass = [6.941,6.941]
charge = [1,0]

properties = {
    "global":{
         "cell":unit_cell,
        "pbc":[True,True,True],
     },
    "sites":{
        "positions":atomic_positions,
        "symbols":symbols,
        "mass":mass,
        "charge":charge,
     }
}

structure = StructureData(
        properties=properties
        )

we can access the properties in this way:

structure.properties.global.cell 
structure.properties.sites.symbols
...

I have the feeling that this way of providing properties is cleaner and that can also make easier the transition of the plugins to the atomistic Data. However I am not sure it is a good alternative.
Each sub-property will be then an attribute of the GlobalProperty or SitesProperty classes. Instead of a list of supported_properties, we can provide a dictionary:

In [1]: structure.properties.get_supported_properties()
Out [1]: {
                  "global":["pbc","cell"],
                   ...
               }

Comments @sphuber , @giovannipizzi , @mbercx ?

from aiida-atomistic.

What is the advantage of classifying the properties according to their "domain"? Is having this knowledge encoded in the property itself necessary? Surely, the developer will know that the cell is "global" and the mass is a site-specific property.

from aiida-atomistic.

I agree that the developer will know the domain of a given property.
And classifying with an additional attribute layer may complicate things.

Having a structure.properties.pbc.domain attribute can be useful if we want to filter only a subset of the properties, and will be just an attribute attached to the class of the property (and will be no stored in the database), so we can still leave it there for now and then remove if we see that is not useful.

Thanks @sphuber .

from aiida-atomistic.

I think using domain is a good design in the long term. The property could be site-specific, or kind-specific, or belong to the structure. By specific the property domain, the code can treat the property differently. For example, when I create a supercell, the code can automatically determine which properties need to be replicated or preserved, based on their domain. This makes the process more straightforward and less error-prone.

Regarding the mass property, one could also define it as a kind-specific property. Maybe some code (e.g., QE) would like a property to be kind-specific, while other codes require the property to be site-specific. Relying on developers to inherently know or remember the domain of each property increases the risk of mistakes.

from aiida-atomistic.

I agree on the domain, in the end it is a cost-zero feature which may turn out to be useful.
About the case of the mass property, I would prefer to leave it as site-based, as all the other domain==intra-site properties. One reason is that in this way we still have a code-agnostic definition of the masses, and the second one is that the kind property will be defined alongside the other (see #4). So in a way we are still using the kinds but at the same time we have a site-based definition.

from aiida-atomistic.

For example, when I create a supercell, the code can automatically determine which properties need to be replicated or preserved, based on their domain.

I think this is a good example. Note that I didn't say that adding this information is not a good idea, I was just asking for use-cases.

I am just wondering about the interface and the requirements/guarantees. Does the internal implementation do anything with the domain information of the property? Or is it solely up to the consumer to use that information?

Regarding the API, I think making the domain available on the property makes sense. Not so sure about making the domain a subnamespace through which to access the property, i.e. node.properties.global.cell is not ideal I would say.

from aiida-atomistic.

For now the implementation just stores the domain information, without using it. The use case of the supercell can be also implemented in the StructureData as a method to return easily a new StructureData node containing the supercell.

About exposing the domain property as attribute in the nesting node.properties.global.cell, I agree that it is not ideal.
Right now I am also concerned about the "value" attribute which allows to access the value of a property:

In [1]: node.properties.pbc.value
Out[1]: [True,True,True]

as you can also see in #9.
because it seems a bit counter-intuitive: a user expects to access the pbc value just accessing node.properties.pbc. The "value" additional layer was provided in case we need also additional parameters for each properties, for example a specific kind detection threshold.
However, we can think to pass kind threshold and other parameters to the constructor.
This will simplify the construction of the properties input dictionary and probably simplify also its modifications:

from aiida-atomistic.