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

Merge AFQMC #1245

Merged
merged 15 commits into from Dec 14, 2018

Conversation

Projects
None yet
5 participants
@markdewing
Copy link
Contributor

markdewing commented Dec 12, 2018

This copies most of the code from #1194

The test directories were not copied, nor the minor update to the manual. Content in OLD and PREVIOUS_VERSION directories were also not copied.

The code was modified from #1194 to work with the updated MPI3 wrapper.

There are two changes to the MPI3 wrapper necessary to build it, included in this PR.

The only changes not in the AFQMC directory should be in QMCMain.cpp and the previously mentioned change in the wrapper.

markdewing added some commits Dec 11, 2018

Copy of AFQMC code
From PR #1194

Directories named 'old' were skipped.
The includes to the mpi3 wrapper have been updated.
Disable shm tests in Numerics
The current code is a timing test, which we don't want running during the unit tests.

Also update the test code for the new mpi3 wrapper.
Update AFQMC unit tests and add is_root
Get the world communicator from OHMMS::Controller.

Add the is_root structure to csr_matrix.hpp and use that one instead of the
one in mpi3/shared_window.hpp.
Convert impl_ access to address-of operator
Also workaround for broadcasting HamiltonianTypes - cast to int, broadcast, cast back.
Comment out static_assert that is failing.
Error says the value is not a constexpr
Two changes to mpi3 wrapper for AFQMC to compile.
Add a move assignment operator to shared_window.
Make all_reduce_value public
@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

By just copying the code aren't you erasing all of the commit history on the AFQMC side?

How can we reproduce calculations we have run in the past without keeping a local backup of this.

@prckent

This comment has been minimized.

Copy link
Contributor

prckent commented Dec 12, 2018

@fdmalone I don't think the goal here is to preserve the history. In any case, for reproducibility you will need to use an archived copy and appropriate commit hash of the older code since the version here could never be run standalone. It will always be linked with new files.

This should not be a concern going forward since merges will occur often, we make versioned releases, and production calculations should use a specific commit hash or (preferred) release.

@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

Ok, but what is the benefit over just merging #1194 and adding Mark's additions on top?

@markdewing

This comment has been minimized.

Copy link
Contributor

markdewing commented Dec 12, 2018

Copying files has a few advantages:

  • It clearly removes files from the repository. There's a 60MB file in the test/ directory that would go into the repository even if removed from the branch (the file size is not huge compared to the repo, but not necessarily small, either)
  • It made it easier to separate the PR into smaller pieces that affected portions of the real-space code. This could also be done with git's cherry-pick and rebase commands, but with a large number of commits it gets unwieldy.

Another problem I'm still thinking through - this is going to create some merge conflicts when Miguel pulls the upstream develop onto his develop.

@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

Yeah that's the bit I'm worried about. I think I will have to rebase heavily.

@markdewing

This comment has been minimized.

Copy link
Contributor

markdewing commented Dec 12, 2018

I'm looking at ways to remove the large file from the branch while preserving more history. I think any technique is going to change the commit hashes and cause merging problems.

@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

Yes, it might just be easier at this point for me to just cherry-pick all of the upstream work onto this once it is merged and start fresh. I think the alternative is too much work at this point and would require going through the pain of rebasing llnl_afqmc onto qmcpack again.

@markdewing

This comment has been minimized.

Copy link
Contributor

markdewing commented Dec 12, 2018

A fresh clone of the qmcpack repo is 458MB. I would like to avoid adding a file that is > 10% of the repo size.

@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

Absolutely. In retrospect I could have rebased onto develop and removed this file, those old directories and isolated the real space work so they could be split into separate commits. Then we could have preserved the commits at least.

@fdmalone

This comment has been minimized.

Copy link
Contributor

fdmalone commented Dec 12, 2018

But I think the best thing to do now is to just go ahead with this merge. I will deal with the pain on our end.

@ye-luo

This comment has been minimized.

Copy link
Contributor

ye-luo commented Dec 12, 2018

@fdmalone Probably this is the best way. Once this gets merged. Everyone working on the AFQMC needs to cherry-pick unmerged changes to the new develop and drop the old one on your side. As @prckent said, once AFQMC development stays close to the main QMCPACK process. No problem should occur again.

@grahamlopez

This comment has been minimized.

Copy link
Contributor

grahamlopez commented Dec 13, 2018

@fdmalone When you are resolving conflicts with your merge/rebase, keep in mind git checkout --ours <path> and git checkout --theirs <path> so that you don't have to manually edit files where you know that you want to keep one or the other as is.

@prckent

This comment has been minimized.

Copy link
Contributor

prckent commented Dec 14, 2018

This uses boost/hana.hpp which is only avail in boost 1.61.0+. We should check the boost version on configure and update our minimum version.

@prckent prckent added this to the V3.6.0 Release milestone Dec 14, 2018

@ye-luo

This comment has been minimized.

Copy link
Contributor

ye-luo commented Dec 14, 2018

All 8 CI builds checked on bora.
gcc 6.2 and cuda 10
CPU and GPU with real aos, cplx aos, real soa MP, cplx soa MP
All the complex builds have afqmc enabled.

@ye-luo

ye-luo approved these changes Dec 14, 2018

@ye-luo ye-luo merged commit c4b9065 into QMCPACK:develop Dec 14, 2018

@wafflebot wafflebot bot removed the in progress label Dec 14, 2018

This was referenced Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment