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

Adding CIS, CISD and other options for GAS-ACI calculations #355

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

huangm29
Copy link
Contributor

@huangm29 huangm29 commented Oct 25, 2023

Description

Adding options for initial guesses of ACI using GAS. This PR was previously #341 (comment)

User Notes

  • Adding CIS, CISD, and CID options for ACTIVE_REF_TYPE, which are initial guess of GAS-ACI
  • Adding GAS_REF_COUNT option which can be combined with GAS option of ACTIVE_REF_TYPE to specify a full GAS for initial guess of GAS-ACI
  • Slightly optimize the GAS-ACI code

Checklist

  • Re-enable GASACI-2 caused by previous bug, and also use it to test the GAS-CISD option
  • Added GASACI-6 test case for GAS-CIS option
  • Added GASACI-7 test case for GAS_REF_COUNT option
  • Modified GASACI-3 and GASACI-4 for GAS-CISD options
  • Ready to go!

@@ -78,9 +78,11 @@ def register_driver_options(options):
options.add_double("MS", None, "Projection of spin onto the z axis")

options.add_str(
"ACTIVE_REF_TYPE", "CAS", ["HF", "CAS", "GAS", "GAS_SINGLE", "CIS", "CID", "CISD", "DOCI"],
"ACTIVE_REF_TYPE", "CAS", ["HF", "CAS", "GAS", "GAS_SINGLE", "CIS", "CID", "CISD", "DOCI","GAS_CIS", "GAS_CISD", "GAS_CID"],
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need "GAS_CIS", "GAS_CISD", "GAS_CID" when we already have "CIS", "CID", "CISD"? Can't the user just use these last ones and the code automatically understands if one is using GAS or not? Unless these are supposed to be both valid for GAS (e.g. "CIS" and "GAS_CIS" are both valid and give different results in a GAS computation) then this is potentially a confusing redundancy. Also, I would suggest using "GAS_SINGLE_DET" instead of "GAS_SINGLE".

options_->get_str("ACTIVE_REF_TYPE") == "GAS" or
options_->get_str("ACTIVE_REF_TYPE") == "GAS_CIS" or
options_->get_str("ACTIVE_REF_TYPE") == "GAS_CISD" or
options_->get_str("ACTIVE_REF_TYPE") == "GAS_CID") {
gas_iteration_ = true;
Copy link
Member

Choose a reason for hiding this comment

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

This connects to the point above. The use of GAS should be triggered by the user passing a GAS space, not by choosing an additional keyword.


// gas_single_criterion_.first stores all allowed i,a for alpha electron transition
// from GAS_count_i to GAS_count_a
// For example, the current gas_configuration allows an alpha electron from GAS1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// For example, the current gas_configuration allows an alpha electron from GAS1
// For example, if the current gas_configuration allows an alpha electron from GAS1

@@ -157,32 +168,53 @@ void AdaptiveCI::get_gas_excited_determinants_sr(
}

// Generate aa excitations
for (auto& gas_count : std::get<0>(gas_double_criterion_)[gas_configuration]) {
for (auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :
for (const auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :

@@ -0,0 +1,46 @@
# GAS ACI calculation for core excited state of COO
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# GAS ACI calculation for core excited state of COO
# GAS ACI calculation for core excited state of CO

@@ -154,6 +157,9 @@ class CI_Reference {
/// Build the complete GAS reference
void build_gas_reference(std::vector<Determinant>& ref_space);

/// Build GAS-CIS/CID/CISD reference
void build_gas_ci_reference(std::vector<Determinant>& ref_space);

/// Build single lowest energy state
void build_gas_single(std::vector<Determinant>& ref_space);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void build_gas_single(std::vector<Determinant>& ref_space);
void build_gas_single_determinant(std::vector<Determinant>& ref_space);

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the more verbose version so it's clear what single means. Propagate this change. I also see, the name of the option makes sense but it is not clear on its own.

Comment on lines 1000 to 1002
// for (const auto& det : ref_space) {
// outfile->Printf("\n %s", str(det, nact_).c_str());
// }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// for (const auto& det : ref_space) {
// outfile->Printf("\n %s", str(det, nact_).c_str());
// }

@@ -803,24 +903,21 @@ void AdaptiveCI::get_gas_excited_determinants_core(
}

// Genearte bb excitations
for (auto& gas_count : std::get<1>(gas_double_criterion_)[gas_configuration]) {
for (auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :
Copy link
Member

Choose a reason for hiding this comment

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

Great use of unpacking!

Suggested change
for (auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :
for (const auto& [gas_count_i, gas_count_j, gas_count_a, gas_count_b] :

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.

2 participants