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

MSD arbitrary species input #2935

Merged
merged 18 commits into from
Feb 24, 2021

Conversation

camelto2
Copy link
Contributor

@camelto2 camelto2 commented Feb 18, 2021

Proposed changes

This PR generalizes the multi-slater build path. Previously, everything was hard wired to have alpha and beta electron species. The PR adds a new build path that can handle any number of fermion species to occupy the multi slater determinants. This partially addresses #2698 in that you could now use only "alpha" electrons (useful for spinors) or additional species for other applications.

essentially, the input (if you want variable number of species) becomes
<multidetermiant fast="yes" optimize="no" spo_0="name0" spo_1="name1" ... spo_(N-1)="nameN-1">
where the number of species is determined by the particle set.

and (in the xml example) a similarly modified det list

<detlist size="5" ... nstates="8" nc0="0" ne0="5" nc1="0" ne1="2" nc2="0" ne2="4" ...>

and the CI specifcation

<ci ... qchem_coeff="-0.8586" occ0="11111000" occ1="00101000" occ2="01101010"/>

The changes are similar for the hdf5 input style.

This keeps all backwards compatibility, so all current unit tests/input will continue to work.

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

  • New feature

Does this introduce a breaking change?

  • No.
    The new input style works and preserves the previous approach so all previous calculations/formats behave as expected. However, there does need to be an additional PR to properly generalize the MultiSlaterDeterminantFast evaluation routines to handle arbitrary sum_i c_i x (an arbitrary product of determinants). See MSD arbitrary species evaluation (1/N) #2947 which addresses the changes necessary for MultiSlaterDeterminantFast.

What systems has this change been tested on?

My workstation

Checklist

  • 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
  • No. Documentation has been added (if appropriate)

@qmc-robot
Copy link

Can one of the admins verify this patch?

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.

Hopefully my reaction is not totally a surprise.

  1. Once Spin-orbit input logic simplification #2735 is solved
    I don't see why we need a new input tag ngroups. I mean ngroups can be figured out by query the electron particle set.

  2. Also the old builder code-path should converge with the new code path, convince me if you really think it is needed to keep two versions. It seems the new code is a good move but we need to kill the old one. Also need input tests like test_spo_collection_input_LCAO_xml.cpp and test_spo_collection_input_MSD_LCAO_h5.cpp.

@camelto2
Copy link
Contributor Author

camelto2 commented Feb 18, 2021

Hopefully my reaction is not totally a surprise.

  1. Once Spin-orbit input logic simplification #2735 is solved
    I don't see why we need a new input tag ngroups. I mean ngroups can be figured out by query the electron particle set.

Sure that can be done. I was essentially just doing a first pass to get some comments.

  1. Also the old builder code-path should converge with the new code path, convince me if you really think it is needed to keep two versions. It seems the new code is a good move but we need to kill the old one. Also need input tests like test_spo_collection_input_LCAO_xml.cpp and test_spo_collection_input_MSD_LCAO_h5.cpp.

I don't think we need to support both paths once the evaluation and everything is cleaned up. I separated everything for now just to make sure I didn't break anything that currently worked.

I will add unit tests once the input style is settled

@ye-luo
Copy link
Contributor

ye-luo commented Feb 18, 2021

@camelto2 I think we don't need to wait for the fix on the evaluation side, as long as the generic input parsing can support the legacy parsing, then it is good enough for merging with some safeguard.
our input class do support multiple tags mapped to the same variable.
mparam.add(occ, "occ1") which is handled by the loop
mparam.add(occ, "occ_up") which can be added for backward compatibility.

@camelto2
Copy link
Contributor Author

@camelto2 I think we don't need to wait for the fix on the evaluation side, as long as the generic input parsing can support the legacy parsing, then it is good enough for merging with some safeguard.
our input class do support multiple tags mapped to the same variable.
mparam.add(occ, "occ1") which is handled by the loop
mparam.add(occ, "occ_up") which can be added for backward compatibility.

Ahh, I didn't think about that. I can update that accordingly to take care of this backward compatibility issue

@ye-luo
Copy link
Contributor

ye-luo commented Feb 18, 2021

Also ParticleSet::groups() returns how many kinds of quantum particles are there.

@camelto2
Copy link
Contributor Author

I removed the need for separate build paths for the MSD fast algorithm. In cases where the number of species is two, I also allow for backwards compatibility. For example, in the hdf5 for two particle species, most hdf5 files will have "CI_Alpha" "CI_Beta". So if reading CI_0, CI_1 fails, it tries CI_Alpha and CI_Beta. Same in the xml path.

I still have overloaded readDetList and readDetListH5, which is used for the legacy all_determinants algorithm, but that can be changed as well using the newer builder functions. Updating that now

@camelto2
Copy link
Contributor Author

camelto2 commented Feb 22, 2021

I added a unit test for the xml style MSD input, with an arbitrary number particle species to make sure everything in constructed correctly. I also removed the need for overloaded readDetList and readDetListH5 so that we can reduce the amount of basically redundant code. Backwards compatibility is retained for both MultiSlaterDeterminant and MultiSlaterDeterminantFast.

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.

Great simplification. Seems only minor issues left.

src/QMCWaveFunctions/Fermion/SlaterDetBuilder.cpp Outdated Show resolved Hide resolved
app_error() << "In SlaterDetBuilder: SPOSet \"" << spo_beta_name
<< "\" is not found. Expected for MultiSlaterDeterminant.\n";
abort();
spos[grp] = sposet_builder_factory_.getSPOSet(spoNames[grp]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The pointer return from getSPOSet should be treated as a reference. It was kept pointer for checking nullptr. Please assign the value to a temporal pointer variable. In the current way, the SPOSet actually got destroyed when you reset the smart pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I'm confused as to what you mean. The smart pointer spo_clones[grp] calls reset, but the argument is spos[grp]->makeClone(). So the spo_clones[grp] should take ownership of the pointer being returned by makeClone(), not taking ownership of spos[grp] itself. So I don't see how the original ptr returned by getSPOSet (spos[grp]) gets destroyed.

Copy link
Contributor

@ye-luo ye-luo Feb 22, 2021

Choose a reason for hiding this comment

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

reset() first calls delete on the previously stored pointer before taking the new one. This is not desired.

Copy link
Contributor Author

@camelto2 camelto2 Feb 23, 2021

Choose a reason for hiding this comment

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

Sure, but spo_clones[grp] was initially a nullptr, so it wouldn't actually call the delete. I guess reset implies that it was storing something before, so it is not as clear as it could be. I changed it to be a bit more clear hopefully.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was confused by spos and spo_clones. Could you remove spos and store the return of sposet_builder_factory_.getSPOSet in spo_tmp for nullptr check locally.

spo_clones.emplace_back(spo_tmp->makeClone());

should be OK.

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

src/QMCWaveFunctions/Fermion/SlaterDetBuilder.cpp Outdated Show resolved Hide resolved
@camelto2 camelto2 changed the title [WIP] MSD arbitrary species input MSD arbitrary species input Feb 23, 2021
@ye-luo
Copy link
Contributor

ye-luo commented Feb 23, 2021

deterministic-unit_test_wavefunction_sposet fails now.

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.

All look good to me.

@ye-luo
Copy link
Contributor

ye-luo commented Feb 24, 2021

Okay to test

@ye-luo ye-luo merged commit 63fb63d into QMCPACK:develop Feb 24, 2021
@ye-luo
Copy link
Contributor

ye-luo commented Feb 25, 2021

@camelto2 camelto2 deleted the MSD_arbitrary_species_input branch September 12, 2022 18:21
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