aiida-vasp / aiida-vasp

A plugin to AiiDA for running simulations with VASP
https://aiida-vasp-plugin.readthedocs.org/en/latest
Other
48 stars 45 forks source link

pressure and other terms missing in the total energies returned #524

Closed zhubonan closed 2 years ago

zhubonan commented 3 years ago

Please check these boxes before submitting your issue, thank you!

Environment

Steps that lead to the error

Run a calculation with PSTRESS.

What should happen

The calculation finishes and the final energies are parserd by aiida-vasp. Note that the "final energy" is not the same as the "final enthalpy", the late has the P * V term included in the energy.

What happened instead

Only the total energy of the system is parsed (e.g energy_extrapolated), the enthalpy is not parsed and saved into the database.

I have checked the code in parsevasp - the final enthalpy is actually parsed. It is the

For a calculation with external pressure, this would give the enthalpy of the system (parser is a parservasp.vasprun.Xml object)

In [53]: parser.get_energies('all', ['energy_free'])
Out[53]:
{'energy_free_final': array([-108.72194243]),
 'energy_free': array([-125.12598088]),
 'electronic_steps': array([1])}

the energy without entropy:

In [40]: parser.get_energies('all', ['energy_no_entropy'])
Out[40]:
{'energy_no_entropy_final': array([-108.72193078]),
 'energy_no_entropy': array([-125.12595757]),
 'electronic_steps': array([1])}

also the extrapolated 0K energy:

parser.get_energies('all', ['energy_extrapolated'])
Out[41]:
{'energy_extrapolated_final': array([-2.331e-05]),
 'energy_extrapolated': array([-125.12596923]),
 'electronic_steps': array([1])}

In these cases, only the _final energy terms include the P*V term.

For ionic step energies (_final), VASP 5.4.4 has a different mapping in the XML output (BUG) for the ionic step:

However, it is fixed in VASP 6.2.0:

At the moment, the parser uses the energies from the electronic step, which has the correct mapping in both VASP 6.20 and VASP 5.4.4. However, the problem is that if any additions to the free energy (TOTEN) is made at the ionic step, like the P*V term and other correction schemes (dispersions etc.), exists the final energy of the system is not correct.

I think we should change the code in aiida-vasp to add the output of the ionic step energies and recover the correct naming and maintain backward compatibility. My proposed change is:

espenfl commented 3 years ago

@zhubonan Yes, this was already prepared for in parsevasp. Thanks for the suggestion and the PR related to this. In fact, for this precise reason I did not want to introduce this when adding it in parsevasp. Also, please remember that some have done a bugfix in the source code of 5.x as this bug is fairly well known and have caused issues in automation since quite some time. As such we should be crystal clear here, otherwise those with a bugfix will obtain the wrong results, even though they are confident it should be right. This concerns both regular calcs and imports, and they need to be consistent. Total energies are also a widely used property.

Before deciding on where we should go with this I think we should decide first on how we approach bugs in the VASP code. I see two ways:

Both approaches as pros and cons and I can see arguments for going both directions. But, assuming that the bugs in the VASP source code is rather limited. At least that is indeed the case for the known bugs it might make more sense to go with the first option and make sure we document it properly.

If we go with the first route we should of course make sure we document it very clearly as users might be very confused when they compare AiiDA-VASP results to regular VASP runs for instance. Also, we might consider to add some kind of more explicit marks in the provenance that we have changed the results from VASP. Should we, and how?

I would really like your comments on this. @zhubonan @atztogo @JPchico.

atztogo commented 3 years ago

@espenfl, I don't have any good idea. Not only VASP versions, but also aiida-vasp versions, the combination of results can be different. To clearly mention in which version of aiida-vasp the treatment starts is important, I agree. By the way, I got a report from a phonopy user that the physical unit of hessian has changed from VASP6, see https://github.com/phonopy/phonopy/issues/155. We may have to be careful some other quantities, too.

zhubonan commented 3 years ago

@espenfl I would go for option 1 as well. If someone has modified the source code and applied the (probably custom) bug fixes then they should have more insights into the exact consequences on postprocessing codes. Since there is the provenance, if the incorrect interpretation is made by the plugin they can go back and recheck (e.g. reparse with parsevasp). Option 2 seems to be rather limiting and adds more work to the user, as they have to have their boilierplate code for detecting these well know bugs.

For this particular case, we can do some sanity checks in the plugin. In VASP 5.4.4 the energy_extrapolated_final (as reported by parservasp) is the entropy term and very small. In fact, it should be equal to energy_free - energy_no_entropy (then times the SCALEE). So we can check if it is the case in the plugin and emit a warning if it is not the case.

Of course, we should document the behaviour clearly 😄

JPchico commented 3 years ago

@espenfl I agree that option 1 is probably the best also.

I also agree with the suggestion of @zhubonan of performing the sanity check, of this way one should be able to more or less detect if the user has fixed the bug in their source code and parse accordingly. Of course we cannot do this for every bug present in the VASP source code, but as this one is well known, and easy to check it is perhaps something worthwhile.

The issue of other units raised by @atztogo is worrisome, I do not know if it makes sense to do in VASP, but something that is done in the qiida-quantumespresso is that they also parse the units of the quantities that they are parsing (not all, some such as the energy, etc.) Of this way one could have some logic based on the units (for other workflows which might need to do operations on the data such as phonopy).

espenfl commented 3 years ago

@atztogo @zhubonan @JPchico Great. Then we all agree. I will look and think a bit on this PR Bonan, but I think it is a good approach for this particular case as well.