Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JFRudzinski opened this issue Aug 16, 2024 · 4 comments
Labels
bug Something isn't working question Further information is requested

Comments

@JFRudzinski
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

@JosePizarro3 JosePizarro3 added bug Something isn't working question Further information is requested labels Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants