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

Slight inconsistencies in conversion factors #161

Closed
aiida-bot opened this issue Feb 10, 2016 · 7 comments · Fixed by #3278
Closed

Slight inconsistencies in conversion factors #161

aiida-bot opened this issue Feb 10, 2016 · 7 comments · Fixed by #3278

Comments

@aiida-bot
Copy link

Originally reported by: Tom Daff (Bitbucket: tdaff, GitHub: Unknown)


In aiida/common/constants.py several conversion factors are defined. Most of them follow CODATA 2006, making them compatible with Quantum ESPRESSO, however the Rydberg is slightly different (c.f http://physics.nist.gov/cuu/pdf/all_2006.pdf):

ry_to_ev = 13.6056917253
CODATA:    13.60569193(34)

This leads to slight inconsistencies where the energy and force are converted with ry_to_ev but the stress is converted with ry_si from CODATA.

The difference is small, but the constants should probably strictly adhere to the standard and ensure that standard is recorded somewhere, since it gets updated fairly regularly.


@aiida-bot
Copy link
Author

Original comment by Andrea Cepellotti (Bitbucket: acepellotti, GitHub: cepellotti):


Hi Tom, thanks for your feedback. You are right, we took them from QE, but they are not exactly equal to CODATA2006.
There are some difficulties to address this problem. Each code might use his own definition of fundamental constants. Moreover, also the fundamental constants are updated by CODATA every 4 years. Do you already have an idea on how to address this?

@sphuber
Copy link
Contributor

sphuber commented Dec 18, 2017

The definition of these constants should not be the responsibility of aiida_core but rather of the plugin. We therefore propose that we move the constants from aiida_core to the aiida-quantumespresso plugin. Since all Quantum ESPRESSO specific files have since the opening of this issue already been moved from aiida_core, we only need to move the constants to the new plugin and should add a deprecation message to the aiida.common.constants module. The elements dictionary should remain however and should not be deprecated.

The only constants that is still being used in aiida_core is the bohr_to_ang constant, which is used in the show_mpl_heatmap method of the TrajectoryData class in aiida/orm/data/array/trajectory.py. It is not clear how this should be handled

@sphuber
Copy link
Contributor

sphuber commented Dec 3, 2018

We cannot move these fully yet, as some of the constants are still being used by classes that are still in aiida-core such as StructureData and CifData. Until we move these to a separate aiida-solidstate plugin, we need to keep them in aiida-core

@sphuber
Copy link
Contributor

sphuber commented Aug 27, 2019

In fact, upon checking again just now, only the bohr_to_ang constant was used and only once. I propose we remove the constants (and I inline the bohr_to_ang in the one visualization method of TrajectoryData) and move the constants to aiida-quantumespresso. When moving them, I can also take care potentiallly of the inconsistency. There is a separate issue for this on aiida-quantumespresso.

Apparently, the same constants also already exist in qe-tools. So maybe we should just use those in aiida-quantumespresso instead of copying them there again. Thoughts @giovannipizzi ?

I searched through all the repositories in aiidateam and only aiida-quantumespresso and qe-tools use the constants from aiida.common.constants. I also searched in aiida-vasp and they don't seem to use it. I think it is safe them to remove them from aiida-core.

@giovannipizzi
Copy link
Member

  1. Ok for me to remove the constants
  2. for qe-tools, it does not depend on aiida-quantumespresso so we cannot make this dependency... maybe the opposite? (Leave constants in qe-tools, depend on them?)

@sphuber
Copy link
Contributor

sphuber commented Aug 28, 2019

2\. for qe-tools, it does not depend on aiida-quantumespresso so we cannot make this dependency... maybe the opposite? (Leave constants in qe-tools, depend on them?)

This is exactly what I would do. The only thing is that we then have to be very careful to not change the constants, or at least be aware that the results of aiida-quantumespresso heavily rely on it

@giovannipizzi
Copy link
Member

giovannipizzi commented Aug 28, 2019

Indeed. I think it's ok though, we make sure now that we define them to put the correct values, and then put a comment not to change them as other packages (as aiida-qe) depend on them.

Actually, the best (and what I think we should do) would be to namespace the constants, e.g.

# Comment on where these data come from
CODATA2006 = {
  'bohr_to_ang': xxx,
  ...
}

# Comment on where these data come from
QE64 = {
    'bohr_to_ang': xxx,
    ...
}

We can even start with only one of these dictionaries, but this essentially ensures that (if we define them correctly the first time) we will never have to change them, but rather have a new dictionary and the dependencies will use the correct one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants