nomad-coe / nomad

NOMAD lets you manage and share your materials science data in a way that makes it truly useful to you, your group, and the community.
https://nomad-lab.eu
Apache License 2.0
71 stars 16 forks source link

GULP Parser does not thoroughly parse pGFN-FF force field #64

Closed Andrew-S-Rosen closed 1 year ago

Andrew-S-Rosen commented 1 year ago

Issue

This is a very niche error that I don't expect to be addressed for the JOSS review but wanted to report anyway.

In GULP >= 6.1, they introduced a new class of force fields called (p)GFN-FF. Calculations of this type do not appear to be parsed correctly in NOMAD just yet.

Input:

from nomad.client import parse

archive = parse('gulp.got')
print(archive[0].run[0].calculation[0].energy.total.value)
print(archive[0].run[0].method[0].force_field)

Returns:

None
ForceField()

The archive[0].metadata.domain also returns dft, which seems incorrect since this is a force field.

Test file, which you can keep or do as you wish: gulp.got.txt

Details of Build

System: Ubuntu via WSL

conda create --name joss_test python=3.9
conda activate joss_test
pip install -e .[parsing] # from https://github.com/arosen93/nomad/tree/develop

Note: I am using a fork of the repo because https://github.com/nomad-coe/nomad/issues/44 is preventing me from running the parsers unless I hard-code where the tools.json file is located.

TLCFEM commented 1 year ago

@ladinesa maybe have a look?

ladinesa commented 1 year ago

yes i will certainly, thanks @TLCFEM for tagging.

ladinesa commented 1 year ago

It seems that for this particular calculation, the pGFNFF force field was used and is not currently parsed. I will try to add a support for this. However, since I think this is a specific kind of force field, I would simply store the parameters as code-specific metadata, e.g. x_gulp_pffnff_accuary (@JFRudzinski do you think we can generalize this?). metadata.domain is something that we need deprecate, this was initially intended to differentiate between the dft and experimental data but I think it has lost its meaning. I will open an issue to address this in the gitlab project.

JFRudzinski commented 1 year ago

I don't know what the exact situation is for the GULP or other non-Lammps, non-Gromacs parsers. But generally I think it is safe to say that we do not really have support for force field metadata (or at least we cannot promise that the working parsers are also properly storing the force field).

In the future, the plan is to support full force field storage through collaboration with the OpenKim project. However, they are just now developing support for bonded force fields, so it will still be some time.

Another option in the future will be to store your FF via a custom MD upload to NOMAD. I am working on a schema and parser for this now, but again it will be some time before this is ready.

In the mean time, I agree with you @ladinesa that we should try to grab any easily accessible force field parameters and throw them into code-specific metadata, but I would not spend very much time on this, since IMO it will likely never be used in this way. Rather, I think the most important thing is to just make sure that the parser doesn't break when, e.g., new FFs are added as in this case.

In terms of tagging these as FF or MD calculation, this has really only been done for Gromacs, Lammps, and FHIaims parsers that I know of. @ladinesa we should start to compile a list of all the parsers that support MD simulations and then start to go through systematically and add support.

ladinesa commented 1 year ago

@JFRudzinski thanks for the input. can you perhaps open an issue where we can address your last point?

JFRudzinski commented 1 year ago

Done: https://github.com/nomad-coe/atomistic-parsers/issues/55

ladinesa commented 1 year ago

implemented parsing of pGFN-FF in nomad-coe/atomistic-parsers#54