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

ESHDF python class and GPAW converter #3617

Closed
wants to merge 17 commits into from

Conversation

tiihonej
Copy link
Contributor

Proposed changes

I wrote and added a generic python class to handle ESHDF files for QMCPACK. The class is presently used by a new GPAW converter, which implements tools to either read a GPAW restart file (.gpw) or convert live calculator objects to ESHDF.

The present layout of the code allows the same Python class framework to be used in any representation of the wavefunction and bind to any mean-field code, which currently implements conversion to QMCPACK. This way the maintenance of the ESHDF specification, and also (in principle) most of the existing converters could be centralized on a user-friendly python platform. I'm not proposing to converting each of the present converters to python, but this remains a possible long-term strategy.

The present implementation is work in progress. (Actually, some of the intended usage of GPAW relies on updates that are still underway.) But generally speaking, the wavefunction conversion from GPAW works fine (see #3490) and the remaining big questions consider refactoring, writeup and testing of the code. I'll write some units tests soon, and I appreciate any ideas on how to do it. Also, I need your opinions on the general layout and some reassurance that the project is facing the right direction. What do you think? @prckent ?

Another related but isolated discussion topic will be GPAW integration in Nexus. I'll work on that with @jtkrogel, but generally with the converter framework in place this should not differ much from how PySCF is presently used.

What type(s) of changes does this code introduce?

  • New feature
  • Refactoring (no functional changes, no api changes)
  • Testing changes (e.g. new unit/integration/performance tests)

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

-RHEL 8.4 laptop

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes. This PR is up to date with current the current state of 'develop'
  • No. Code added or changed in the PR has been clang-formatted
  • Not yet. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • No. Documentation has been added (if appropriate)

@qmc-robot
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

Copyright needs to be correct.
Unit tests would help show the input expected by match_gv and desymmetrize_z as well as being good form.

src/QMCTools/Eshdf.py Show resolved Hide resolved
src/QMCTools/Eshdf.py Outdated Show resolved Hide resolved
src/QMCTools/gpaw4qmcpack Outdated Show resolved Hide resolved
src/QMCTools/Eshdf.py Outdated Show resolved Hide resolved
@tiihonej
Copy link
Contributor Author

Thank you @PDoakORNL . I'll make the changes. I assume the best way to make unit tests is to write, for example, tests/converter/gpaw4qmcpack_test.py, which tests the core features and methods, and refer to in in the local CMakeLists.txt. Sounds good?

PDoakORNL
PDoakORNL previously approved these changes Dec 17, 2021
@PDoakORNL
Copy link
Contributor

Sorry for the slow turn around on approving the change. I notice this is still [WIP] other than bringing it up to date with develop are there remaining issues to sort out?

@tiihonej
Copy link
Contributor Author

No worries. The WIP status was intentional as long as the testing side is missing. The latest commits add a test, where I built upon the earlier converter tests and which I tried to make as comprehensive but modestly sized as possible. I also added conditions to find GPAW and ASE before the executable or the tests are added.

There's room for testing other functionality and implementing features for the converter, but I think this one should catch most of the breaking changes and is now ready to be added to main repository as an alpha version.

@tiihonej tiihonej changed the title WIP: ESHDF python class and GPAW converter ESHDF python class and GPAW converter Dec 20, 2021
@tiihonej tiihonej changed the title ESHDF python class and GPAW converter WIP: ESHDF python class and GPAW converter Dec 20, 2021
@tiihonej tiihonej changed the title WIP: ESHDF python class and GPAW converter ESHDF python class and GPAW converter Dec 20, 2021
Copy link
Contributor

@PDoakORNL PDoakORNL left a comment

Choose a reason for hiding this comment

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

This looks solid. The copyright statement should be updated.

src/QMCTools/Eshdf.py Outdated Show resolved Hide resolved
PDoakORNL
PDoakORNL previously approved these changes Dec 21, 2021
@ye-luo ye-luo self-requested a review December 21, 2021 16:06
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

I added many comments but I may misunderstand certain places.

src/QMCTools/CMakeLists.txt Outdated Show resolved Hide resolved
src/QMCTools/gpaw4qmcpack Outdated Show resolved Hide resolved
src/QMCTools/Eshdf_gpaw.py Show resolved Hide resolved
tests/gpaw/CMakeLists.txt Outdated Show resolved Hide resolved
tests/gpaw/test_Si_diamond/run_si.py Outdated Show resolved Hide resolved
tests/gpaw/CMakeLists.txt Outdated Show resolved Hide resolved
tests/gpaw/CMakeLists.txt Outdated Show resolved Hide resolved
@tiihonej
Copy link
Contributor Author

Thanks for the comments, I think they're all on point. I'll make the necessary changes early next year.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

@tiihonej this is my last request. Once you update this, I will approve the PR.

CMakeLists.txt Outdated Show resolved Hide resolved
src/QMCTools/CMakeLists.txt Outdated Show resolved Hide resolved
ye-luo
ye-luo previously approved these changes Jan 25, 2022
Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

LGTM (I mostly looked at the CMake part)

@tiihonej
Copy link
Contributor Author

tiihonej commented Feb 1, 2022

The testing seems stale. I'm closing this PR and making a new one.

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.

4 participants