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

Import physical constants from qe-tools instead of aiida-core #388

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Aug 28, 2019

Fixes #69

The physical constants, mostly used by the parsers, used to be defined
in aiida-core for the Quantum ESPRESSO plugins, before they were moved
to this repository. The physical constants remained, however, but will
be removed for aiida-core==1.0.0. The constants had already be copied
to qe-tools as well, which is where they will stay. Here we change the
import to use the constants from the latter instead of aiida-core.

The physical constants, mostly used by the parsers, used to be defined
in `aiida-core` for the Quantum ESPRESSO plugins, before they were moved
to this repository. The physical constants remained, however, but will
be removed for `aiida-core==1.0.0`. The constants had already be copied
to `qe-tools` as well, which is where they will stay. Here we change the
import to use the constants from the latter instead of `aiida-core`.
@sphuber sphuber requested a review from giovannipizzi August 28, 2019 08:50
@sphuber
Copy link
Contributor Author

sphuber commented Aug 28, 2019

I will open a PR on aiida-core as well to remove the constants from there. I was just thinking about compatibility though. Removing them from aiida-core will break existing aiida-quantumespresso releases. Given that they are alphas, that might be acceptable, especially if we make another release of aiida-quantumespresso, except we just released one yesterday. What do you think @giovannipizzi

@sphuber
Copy link
Contributor Author

sphuber commented Aug 28, 2019

I also opened an issue on qe-tools to continue the discussion on the original problem of inconsistent use of constant sets.

Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok with me. Anyway as I mentioned elsewhere, we should namespace the constants in qetools and get them from the correct namespace. Since the constants are already there anyway, ok for now as it is.
I think that when we change qe-tools, we will release a new version of it, and a new one of aiida-qe depending on it.

For the dependency of aiida-qe on aiida-core: the latest released version of aiida-qe should work with the latest released version of aiida-core. Also the develop versions should work together. Probably this is enough, but if it isn't, then we'll need to make a new release for both as soon as the change is applied? (anyway, maybe it's not critical, since in the new develop of aiida-qe we are not taking them from aiida-core anymore?)

@sphuber
Copy link
Contributor Author

sphuber commented Aug 29, 2019

The only problem is that I should release a new alpha of aiida-quantumespresso that depends on qe-tools for the constants before a release is made of aiida-core where they are removed. Otherwise, if people upgrade their aiida-core, their current aiida-quantumespresso will fail. That is why I blocked the PR on aiida-core until after I made the latest beta release. That way we will have some period of leeway where we can apply the changes in aiida-quantumespresso and make yet another release there as well. It is unfortunate we just released an alpha on monday evening

@sphuber sphuber merged commit b1f44a4 into develop Aug 29, 2019
@sphuber sphuber deleted the fix_068_change_units_import branch August 29, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent usage of conversion factors
2 participants