nomad-coe / electronic-parsers

Apache License 2.0
18 stars 7 forks source link

openmx: parse vdW info #196

Closed ondracka closed 4 months ago

ondracka commented 5 months ago

Parse the info about used van der Waals correction method with test.

I used G10 key for the DFT-D3 method (S. Grimme, J. Antony, S. Ehrlich, and H. Krieg, J. Chem. Phys. 132, 154104 (2010)), it is not defined in the metainfo, but at least one parser already uses it and it is consistent with the G06 definition).

However I have to say I'm not too happy about the metainfo shortcuts (for the Grimme's methods), to be honest I've never seen G06 or G10 used as a shortcut for them, while in contrary DFT-D2 and DFT-D3 (and recently DFT-D4) seems to be the defacto standard nomenclature.

For example: https://www.chemie.uni-bonn.de/grimme/de/software/dft-d3 (this is the Grimme's page) https://www.vasp.at/wiki/index.php/Category:Van_der_Waals_functionals https://www.quantum-espresso.org/Doc/INPUT_PW.html#idm656 https://www.openmx-square.org/openmx_man3.9/node164.html https://wiki.fysik.dtu.dk/ase/ase/calculators/dftd3.html etc...

I plan to add support for this to more parsers as I have some plans for some data mining from NOMAD where it would be very usefull if I could tell if vdW correction was used or not, so now might be a good time to open the discussion about the metainfo clarification...

Also "none" or "no" instead of empty string might be a better value for the case when no correction was applied?

ondracka commented 4 months ago

ping @ladinesa @ndaelman-hu as possible reviewers

ndaelman-hu commented 4 months ago

@ondracka we are also trying out Discord now as the new place of community support: https://discord.gg/q9bg7DD6 Obviously, technical discussions about an issue can remain on the Git repo :)

ndaelman-hu commented 4 months ago

Also "none" or "no" instead of empty string might be a better value for the case when no correction was applied?

It should be None, I agree.

Moreover, the name VdW should be changed to clarify when we mean an external correction or the physics of the Hamiltonian. @JosePizarro3 We should look into an approach for this when designing ModelMethod.

ndaelman-hu commented 4 months ago

@ondracka The workflow doesn't seem to have triggered. Did you try the tests locally?

ondracka commented 4 months ago

Hey! There has been some backlog with VdW, so this is a great way to advance it.

I'm mostly looking at this from the schema perspective. We are migrating the schemas to data, after which I could make a thesarus of VdW terms. Let me know if you want to contribute there.

No need to ping me, the rest of the current definitions look good anyway, the Grimme's methods (and maybe the empty string for no vdW correction) are the only cases I disliked.

Regarding tests, as we move toward more standalone sections with their own normalizers, I'd move towards unit testing, i.e. feeding smaller chunks of more varied cases rather than full calculations. This is to improve test coverage while reducing their size. Any opinions @ladinesa ?

So should I reduce this new testcase? BTW in the past I always commited full calculations so that we test in same scenario as during upstream upload and also to save myself some work in the future (so that I don't have to add new testcase for every new feature, sometimes the new tests could just reuse the calculation data that is already in the repo). Just let me know how to do it properly.

ondracka commented 4 months ago

@ondracka The workflow doesn't seem to have triggered. Did you try the tests locally?

I think it was because the upstream moved in the meantime, it runs fine after rebasing...

ndaelman-hu commented 4 months ago

So should I reduce this new testcase? BTW in the past I always commited full calculations so that we test in same scenario as during upstream upload and also to save myself some work in the future (so that I don't have to add new testcase for every new feature, sometimes the new tests could just reuse the calculation data that is already in the repo). Just let me know how to do it properly.

Let's see what @ladinesa says. Probably this is best for when we move to the standalone sections. In that case, we will reduce them later on.

I understand reusing tests for features, but in many cases new calculations have to be generated anyhow, meaning the work and volume is bigger anyhow.

ondracka commented 4 months ago

OK, sorry, might have been too hasty with the merge button when I saw the approval.

ndaelman-hu commented 4 months ago

OK, sorry, might have been too hasty with the merge button when I saw the approval.

It's fine. No worries. Thx for contributing!

ondracka commented 4 months ago

Thanks for the review.