PhysicalProperty rank checks broken

[ndaelman-hu]

While testing MappingAnnotation, the ValueError in physical_property.py, line 257 gets raised due to n_bands = None. I have 2 major issues with this check:

1. triggered at instantiation: this should be a check during normalization. There are plenty of cases where the quantity may be set first at normalization time, if not by the section itself, then by another.
2. clashing namespace: the shorthand of checking quantities starting with n_ is inappropriate, as it lays claim to these naming formats, nor is this limitation ever documented. I would opt for a more flexible system of guarding quantities. Even if we stay with a reserved namespace, it should be chosen more specifically to PhysicalProperty.

@JFRudzinski could we reformulate this check to the requirements of physical property? Else, I recommend removal.

        # Checking if the quantities `n_` are defined, as this are used to calculate `rank`
        for quantity, _ in self.m_def.all_quantities.items():
            if quantity.startswith('n_') and getattr(self, quantity) is None:
                raise ValueError(
                    f'`{quantity}` is not defined during initialization of the class.'
                )

[JosePizarro3]

@ladinesa brought this up in #129. I think he had a fix.

In any case, rank needs to be clarified with the interoperability task force. @lauri-codes @markus1978 please, bear in mind our cases where rank is defined at parsing/normalization time, like it happens when a property is a tensor of order some degree of freedom n_xxx (spin, orbitals, etc.). Maybe you have a better idea of how to define such properties with our current idea of rank.

[ndaelman-hu]

@ladinesa brought this up in #129. I think he had a fix.

In any case, rank needs to be clarified with the interoperability task force. @lauri-codes @markus1978 please, bear in mind our cases where rank is defined at parsing/normalization time, like it happens when a property is a tensor of order some degree of freedom n_xxx (spin, orbitals, etc.). Maybe you have a better idea of how to define such properties with our current idea of rank.

Thx for brining up Alvin. I spoke with him and indeed has a quick workaround, but it comes with unacceptable losses in performance. He agrees that the solution should lie with PhyiscalProperty.

Maybe you have a better idea of how to define such properties with our current idea of rank.

Well, I stated my opinion above. I'd need to understand better the aim to steer it by.

[JFRudzinski]

@ndaelman-hu yes, I also agree that it should be addressed within PhysicalProperty. I do not have any specific suggestions atm, I need to also look into it more.

[JosePizarro3]

Problem comes when defining the shape of value, it will have troubles if you set the attribute of the same and it has a non-zero rank. So normalization cannot handle this, or at least, we should think on another way.

[ndaelman-hu]

Problem comes when defining the shape of value, it will have troubles if you set the attribute of the same and it has a non-zero rank. So normalization cannot handle this, or at least, we should think on another way.

How about only enforcing shape then? You can still extract this using normalization.
In the current scheme, the n_ quantities are more so "nice to have", but far from crucial. Hence, they do not warrant this check.

[JosePizarro3]

How about only enforcing shape then?

This is precisely what we want to avoid, because of the use of variables.

[JosePizarro3]

Maybe these cases where rank needs to have an n_xx quantity defined should be moved into Variables?

[ndaelman-hu]

Maybe these cases where rank needs to have an n_xx quantity defined should be moved into Variables?

That sounds better, yes. Still, why bother with n_ quantities to be set? You already have n_points (again, provoking potential name clashes) in Variables. You can just read the dimensionalities from there and compute shape. Any section inheriting from Variables should just populate n_points.

[ndaelman-hu]

Looking more at the PhysicalProperty code, this seems to be exactly what validate_quantity_wrt_value already does.
It appears that this decorator was implemented later on, as a generalization to n_ handling.