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

Add support for OpenMX VPS format #44

Merged
merged 5 commits into from
Feb 22, 2021
Merged

Conversation

zooks97
Copy link
Contributor

@zooks97 zooks97 commented Feb 10, 2021

I'm working on implementing an AiiDA plugin for the O(N)/local numerical pseudo atomic orbital basis DFT code OpenMX over in aiida-openmx, so I've added support for their rather eclectic VPS pseudopotential format.

The element / valence / exchange-correlation parsing is rather brittle as a general rule because it's all based on the copied input file that they include as a header to all their VPS files. I'm not sure if their pseudopotential code puts this in by itself or if that's just how they prepare the files for release.

As far as I can tell, there is no information about the atomic species in the actual pseudopotential block at the bottom of the VPS files, so this is the best way I've found to get it.

I think that personally-generated pseudopotentials for this code are so niche that any problems that arise can be dealt with when / if they do.

I didn't add any families / CLI installer for these because the pseudos are only available bundled with the source, and I haven't taken the time to codify the suggestions table that they provide.

@zooks97 zooks97 requested a review from sphuber February 10, 2021 18:50
@zooks97
Copy link
Contributor Author

zooks97 commented Feb 10, 2021

I didn't change the version in the commit, but it would be nice to have at least a minor version bump with this.

@zooks97
Copy link
Contributor Author

zooks97 commented Feb 18, 2021

Pinging @sphuber : I'm moving towards an aiida-openmx release in the next few days, and I depend on VpsData for the pseudopotentials there.

@sphuber
Copy link
Contributor

sphuber commented Feb 18, 2021

will try to look at it by tomorrow evening

@sphuber
Copy link
Contributor

sphuber commented Feb 22, 2021

@zooks97 thanks, this all looks fine. If you fix the pre-commit I will merge this.

@zooks97
Copy link
Contributor Author

zooks97 commented Feb 22, 2021

I didn't change anything in the file where the pre-commit failed. It says something about using a generator in conftest.py

@sphuber
Copy link
Contributor

sphuber commented Feb 22, 2021

It's because a new version of pylint apparently got released that adds a new check. All you have to do is remove the square brackets from line 145 in tests/conftest.py and it should work.

@zooks97
Copy link
Contributor Author

zooks97 commented Feb 22, 2021

All good!

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @zooks97

@sphuber sphuber merged commit 85988a9 into aiidateam:master Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants