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

Update Quantum Espresso calculator #325

Merged
merged 22 commits into from
Jul 30, 2024
Merged

Update Quantum Espresso calculator #325

merged 22 commits into from
Jul 30, 2024

Conversation

gelzinyte
Copy link
Contributor

No description provided.

@gelzinyte
Copy link
Contributor Author

Continuing conversations from #321:

I wanted to test both automatically reading the profile from the config file and explicitly passing it to the wrapper.

In that case, how about having one test that relies on the environment variable and the rest be local-profile-based?

The thing that makes it all tricky is that the tests need to use their own pseudopotentials, but the profile includes information on where the pseudopotential directory is. So you can't just rely on the user's generic profile (unless you copy the testing PPs into their directory). I'll try to document this in the files a bit more carefully

Maybe that's uncceptable, but can we copy the test-pseudopotentials to where the profile points and name it something explicit, e.g. wfl_pytest.Si.pz-vbc.UPF? Also explicitly document this behaviour.

Realistically, it would probably only be developers testing QE locally and I don't mind a file copied (and then deleted?) to where ase config points, because I have a separate just for ase testing.

@bernstei
Copy link
Contributor

Is #325 (comment) still an issue?

I settled on always using the user's default profile (no longer testing passing profile explicitly, since that's really an internal ASE issue), and taking advantage of the fact that you can pass pseudo_dir as an explicit argument to the calculator constructor, which overrides the profile, to make sure it uses the test's PP.

@gelzinyte
Copy link
Contributor Author

Is #325 (comment) still an issue?

No, I think this is all ready to merge, once the the tests pass. Thanks!

@bernstei
Copy link
Contributor

Is #325 (comment) still an issue?

No, I think this is all ready to merge, once the the tests pass. Thanks!

I'll let you merge in the recent merged PR and we'll see what happens here.

@bernstei
Copy link
Contributor

I'm patching up the test failures, but do we even understand how this branch ended up with minima hopping tests that don't quite match the minima-hopping code?

@bernstei
Copy link
Contributor

Looks like the CI still isn't seeing the ASE config to get the profile from. Working on it.

@gelzinyte
Copy link
Contributor Author

I'm patching up the test failures, but do we even understand how this branch ended up with minima hopping tests that don't quite match the minima-hopping code?

@jungsdao has also contributed some minima hopping modifications in #321, which moved over with the branch. @bernstei , @jungsdao are the minima hopping changes also ok to merge to main?

@jungsdao
Copy link
Contributor

It looks good to me :) thanks!

@bernstei
Copy link
Contributor

Are we ready to merge?

@gelzinyte gelzinyte merged commit 5758f8c into main Jul 30, 2024
5 checks passed
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.

3 participants