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 real-space RMG converter to convert4qmc #3351

Merged
merged 22 commits into from
Nov 3, 2021

Conversation

kgasperich
Copy link
Contributor

@kgasperich kgasperich commented Aug 9, 2021

Proposed changes

This PR adds RMG hdf5 output to the list of formats supported by convert4qmc (this hdf5 output is generated when the RMG input contains write_qmcpack_restart = "true").

Handling of pseudopotential files is not perfect, so this should be cleaned up in a later PR (the PP terms in the Hamiltonian in the generated *.qmc.in-wf{j,noj}.xml will always reference X.qmcpp.xml where X is the appropriate element symbol, but convert4qmc will not generate/link/move/copy/convert these from the PP files used by RMG).

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

  • New feature

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

cooley (RHEL 7; x86_64) icc 19.1.0.166

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'
  • Yes. Code added or changed in the PR has been clang-formatted
  • Yes*. This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • Yes. Documentation has been added (if appropriate)

@qmc-robot
Copy link

Can one of the admins verify this patch?

@PDoakORNL
Copy link
Contributor

any tests at all?

@kgasperich
Copy link
Contributor Author

I can add one (or more) to https://github.com/QMCPACK/qmcpack/tree/develop/tests/converter
The smallest ones I have right now (all spin-unpolarized) are 5, 80, 95, and 136 MB (size of the hdf5 output from RMG), and I can add any subset of those.
In terms of coverage, it might be good to have a spin-polarized system, but the smallest of those that I have right now is 1.3 GB; I can try to converge that on a coarser grid to get the size down.

@PDoakORNL
Copy link
Contributor

#3354 is relevant to this. We probably don't want to hold the PR for that issue to be resolved.

@kgasperich
Copy link
Contributor Author

Ok, I added two tests (2.5 and 5 MB), one spin-polarized and one unpolarized.
We can move things around as necessary depending on how #3354 is resolved

Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

Thanks Kevin.

Questions on the test choices:

what does the k-point grid in the diamond test case achieve? can we save some filespace by using 1x1x1?

for the o atom case, can we make it even more distinct? it is spin polarized but can we readily support open boundary conditions? (relatively unique feature of rmg, hits more code paths)

@prckent
Copy link
Contributor

prckent commented Aug 19, 2021

Start testing in-house

@prckent
Copy link
Contributor

prckent commented Aug 20, 2021

Test this please

@kgasperich
Copy link
Contributor Author

kgasperich commented Aug 25, 2021

@prckent

what does the k-point grid in the diamond test case achieve? can we save some filespace by using 1x1x1?

In the interest of expediency I was just selecting a subset of the existing RMG example tests; I wanted to use the O atom for a spin-polarized example, and because that was 1x1x1 I thought it might provide better coverage to use a larger grid for the spin-unpolarized test (diamond). The diamond test uses a 4x4x4 grid, but I can change that to 1x1x1 if the grid isn't important (or to 1x1x2 or 2x2x2 if we want something smaller than what we have, but larger than 1x1x1).

for the o atom case, can we make it even more distinct? it is spin polarized but can we readily support open boundary conditions? (relatively unique feature of rmg, hits more code paths)

I'm not sure I understand the question; I was under the impression that RMG only supported periodic boundary conditions.
https://github.com/RMGDFT/rmgdft/blob/606ae210d50b1bf76e228ff98f1a9058e560c499/Input/ReadCommon.cpp#L423-L426
Should I add an option to convert4qmc to generate inputs with bconds set to n n n?

@prckent
Copy link
Contributor

prckent commented Sep 2, 2021

Use the smallest grid for file size. Forget my comments on non-periodic settings.

@kgasperich
Copy link
Contributor Author

@prckent I added some ctest framework and a working example for RMG. I still need to figure out how/where to set the path to the RMG executable (maybe best to just add a cmake variable RMG_BIN analogous to QE_BIN) and I'll need to add some logic so that the RMG tests are added only if RMG is found (something similar to FindQE.cmake).

@kgasperich
Copy link
Contributor Author

@prckent the RMG testing should be working now as long as RMG_BIN is passed to cmake. Should we add an RMG build step to the nightly test scripts?

@prckent
Copy link
Contributor

prckent commented Sep 29, 2021

Is it safe to use v4.2.2 of RMG or do we need to use develop?

@prckent
Copy link
Contributor

prckent commented Oct 15, 2021

This is now working for me. Will have minor comments only.

@prckent prckent self-requested a review October 18, 2021 17:48
Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

  1. Please can you update the logging so that it only prints "Adding RMG tests" as we do with other test categories?
-- Adding integration tests for QMCPACK
Use --log-level=VERBOSE CMake option for details of which tests will be enabled.
Adding estimator tests for QMCPACK
Adding I/O tests for QMCPACK
Adding performance tests for QMCPACK
Adding tests for rmg -> convert4qmc -> QMCPACK workflow
workdir: /home/pk7/projects/qmc/git_QMCPACK_prckent/build_quick/tests/rmg/rmg-Diamond2-1x1x1-gamma-ccECP-np-1
rmg-Diamond2-1x1x1-gamma-ccECP-np-1
adding RMG tests
Adding system tests for QMCPACK
Python dependencies met. Adding PySCF workflow tests
Adding example tests for QMCPACK
Adding Nexus tests
-- Configuring done
-- Generating done
-- Build files have been written to: /home/pk7/projects/qmc/git_QMCPACK_prckent/build_quick
  1. Not critical but was there a reason to not repurpose an existing pseudopotential file and use your C.qmcpp.xml ?
  2. Add RMG_BIN in top level readme.md

// This file is distributed under the University of Illinois/NCSA Open Source License.
// See LICENSE file in top directory for details.
//
// Copyright (c) 2016 Jeongnim Kim and QMCPACK developers.
Copy link
Contributor

Choose a reason for hiding this comment

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

2021

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

RMGParser(int argc, char** argv);

std::vector<std::string> ECP_names;
//std::vector<std::string> tags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented lines, including those below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@prckent
Copy link
Contributor

prckent commented Oct 18, 2021

Working

pk7@sulfur build_quick]$ ctest -R rmg --output-on-failure
Test project /home/pk7/projects/qmc/git_QMCPACK_prckent/build_quick
      Start   62: converter_test_diamond2_rmg
 1/15 Test   #62: converter_test_diamond2_rmg ............................   Passed    0.39 sec
      Start   63: converter_test_O_rmg
 2/15 Test   #63: converter_test_O_rmg ...................................   Passed    0.36 sec
      Start  228: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-scf
 3/15 Test  #228: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-scf ................   Passed    6.70 sec
      Start  229: LINK_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_h5_Waves
 4/15 Test  #229: LINK_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_h5_Waves ......   Passed    0.05 sec
      Start  230: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-rmg2qmc
 5/15 Test  #230: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-rmg2qmc ............   Passed    0.29 sec
      Start  231: LINK_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_TO_1-16
 6/15 Test  #231: LINK_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_TO_1-16 .......   Passed    0.05 sec
      Start  232: COPY_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_XML_TO_1-16
 7/15 Test  #232: COPY_rmg-Diamond2-1x1x1-gamma-ccECP-np-1_XML_TO_1-16 ...   Passed    0.08 sec
      Start  233: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16
 8/15 Test  #233: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16 ...............   Passed    7.54 sec
      Start  234: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-kinetic
 9/15 Test  #234: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-kinetic .......   Passed    0.07 sec
      Start  235: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-samples
10/15 Test  #235: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-samples .......   Passed    0.07 sec
      Start  236: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-localecp
11/15 Test  #236: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-localecp ......   Passed    0.07 sec
      Start  237: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-nonlocalecp
12/15 Test  #237: rmg-Diamond2-1x1x1-gamma-ccECP-np-1-1-16-nonlocalecp ...   Passed    0.07 sec
      Start 2034: ntest_nexus_rmg_input
13/15 Test #2034: ntest_nexus_rmg_input ..................................   Passed    0.71 sec
      Start 2043: ntest_nexus_rmg_analyzer
14/15 Test #2043: ntest_nexus_rmg_analyzer ...............................   Passed    0.58 sec
      Start 2051: ntest_nexus_rmg_simulation
15/15 Test #2051: ntest_nexus_rmg_simulation .............................   Passed    0.56 sec

100% tests passed, 0 tests failed out of 15

@prckent
Copy link
Contributor

prckent commented Oct 18, 2021

We could discuss the diamond test names as well: Here they are "Diamond2". Elsewhere we have "diamond" and "diamondC". In the performance tests we also have atom and electron counts indicated, which is certainly useful.
Perhaps use diamondC-a2-e8, or is this overkill? We could update the other test names consistently over time (not here).

Edited to add: I suggest to change to just diamondC for now. Otherwise the names are unwieldy.

# Locate rmg-cpu
# Take RMG_BIN as hint for location

find_path(RMG_CPU_DIR rmg-cpu HINTS ${RMG_BIN})
Copy link
Contributor

Choose a reason for hiding this comment

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

find_program(RMG_CPU_EXE rmg-cpu HINTS ${RMG_BIN}) and use RMG_CPU_EXE in tests are more straight forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that does look cleaner; I've changed it

@prckent
Copy link
Contributor

prckent commented Nov 3, 2021

@kgasperich What are your plans to update this? I would really like to get it merged and in use in our nightlies. Additional updates can be done in later PRs. The nightly scripts are updated and rmg installed, so these tests should start to run once this is merged.

@kgasperich
Copy link
Contributor Author

kgasperich commented Nov 3, 2021

@prckent sorry, it fell off my radar after the last meeting, but I've updated it now and brought it up-to-date with develop.
The only issues mentioned above that I haven't addressed yet are the test names and the reuse of pseudopotential files

@prckent
Copy link
Contributor

prckent commented Nov 3, 2021

As just discussed at a meeting on this, the updates will be handled in later PRs so that we can get this in use today.

@prckent
Copy link
Contributor

prckent commented Nov 3, 2021

Start testing in-house

@ye-luo
Copy link
Contributor

ye-luo commented Nov 3, 2021

Test this please

@ye-luo
Copy link
Contributor

ye-luo commented Nov 3, 2021

I saw binary h5 files are added but we also have ways to generate them. Can we remove those h5 and bake converter test into the workflow test? If we are in favor of merging this as it is. Need to squash to make sure no intermediate files (I saw some in the history) stored in the repo. When these h5 files need updating, they should just be removed.

@ye-luo ye-luo enabled auto-merge (squash) November 3, 2021 20:41
@prckent
Copy link
Contributor

prckent commented Nov 3, 2021

I think this is consistent with our QE tests. We can't test the converter without pregenerated files in the repo.

@ye-luo ye-luo merged commit 2c533a4 into QMCPACK:develop Nov 3, 2021
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.

5 participants