search

nomad-coe / nomad-simulations

A NOMAD plugin containing base sections for simulations.
https://nomad-coe.github.io/nomad-simulations/
Apache License 2.0
4 stars 1 forks source link

Variables not included for properties when class is initialized in parent normalization #108

Open JFRudzinski opened 4 weeks ago

JFRudzinski commented 4 weeks ago

This occurred in the energies example in Part 4 of Tutorial 14, with the relevant code from the schema solution within the vasp-plugin.

Here the UnknownEnergy class is added to total energy contributions in the normalizer of TotalEnergy(). The idea was for this to always happen, even if the parser does not add this class to the archive. However, in this case the normalization of the child class is not run.

Do we have a general solution to this?

JosePizarro3 commented 4 weeks ago

Sadly, there is no solution. Thing is that normalize() is only run if the class has been instantiated and appended, otherwise it won't.

The logic is a bit convoluted: if the parser does not add it, it is easy to instantiate UnknownEnergy and call for its normalize() function in TotalEnergy().normalize(). However, if the class is instantiated before and appended, this will make the normalize to run twice.

The solution might be to define a TotalEnergy.m_cache flag to run or not the normalization of UnknownEnergy in TotalEnergy, so the parser has to set this. Something like (in the parser):

unknown_energy = UnknownEnergy()
unknown_energy.m_cache['normalize'] = false

and in the TotalEnergy.normalize function:

if self.contributions is None:
    contribution = UnknownEnergy()
    contribution .m_cache['normalize'] = true
    self.contributions.append(contribution)
if contributions is not None and isinstance(contributions.m_def, UnknownEnergy) and contributions.m_cache['normalize']:
    contributions.normalize(archive, logger)

@ladinesa what do you think?

ladinesa commented 3 weeks ago

not sure if I understood the problem fully but I think this is one of the cases where a separate normalizer class is really necessary. One can also leverage the after_normalization abstract method of a parser.

JosePizarro3 commented 3 weeks ago

I'd kind of avoid having yet another layer of normalizations on top. After the weekend, I am pretty convinced that this will be about checking if UnknownEnergy() exists under contributions, and if not, create it and call for normalize().

Something similar happens for Symmetry and ModelSystem: https://github.com/nomad-coe/nomad-simulations/blob/develop/src/nomad_simulations/schema_packages/model_system.py#L1027 (btw I just realized there is a bug there, it should be if self.type=='bulk' and not self.symmetry:).

JFRudzinski commented 3 weeks ago

I'd kind of avoid having yet another layer of normalizations on top. After the weekend, I am pretty convinced that this will be about checking if UnknownEnergy() exists under contributions, and if not, create it and call for normalize().

Something similar happens for Symmetry and ModelSystem: https://github.com/nomad-coe/nomad-simulations/blob/develop/src/nomad_simulations/schema_packages/model_system.py#L1027 (btw I just realized there is a bug there, it should be if self.type=='bulk' and not self.symmetry:).

Ok, I see. I will try to implement this when I have a chance.