Skip to content

Doc: modify input parameters for symmetry and Exx#2503

Merged
dyzheng merged 13 commits intodeepmodeling:developfrom
maki49:symmetry_fix
May 25, 2023
Merged

Doc: modify input parameters for symmetry and Exx#2503
dyzheng merged 13 commits intodeepmodeling:developfrom
maki49:symmetry_fix

Conversation

@maki49
Copy link
Collaborator

@maki49 maki49 commented May 23, 2023

@maki49 maki49 changed the title set the default value of symmetry to 1 Doc: modify input parameters for symmetry and Exx May 23, 2023
Co-authored-by: dyzheng <zhengdy@dp.tech>
@@ -1723,30 +1726,34 @@ These variables are relevant when using hybrid functionals
### exx_hse_omega

- **Type**: Real
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe Availability can be added in front of "### exx_hybrid_alpha"

@maki49 maki49 requested a review from dyzheng May 23, 2023 09:49
@maki49 maki49 marked this pull request as draft May 23, 2023 12:20
@maki49 maki49 marked this pull request as ready for review May 23, 2023 16:20
@Satinelamp
Copy link

Satinelamp commented May 23, 2023

This is an excellent feature to let ABACUS decide how to set symmetry. But using a string type of symmetry parameter seems not necessary, you could provide a new integer option, e.g. symmetry=3, and use the following code to set symmetry to 0 or 1

    if (symmetry == 3)
    {   //deal with no-forced default value
        if (calculation == "nscf" || calculation == "get_S" || calculation == "istate" || calculation == "ienvelope")
            symmetry = 0;     //if md or exx, symmetry will be force-set to 0 or -1 later
        else
            symmetry = 1;
    }

the usage of std::stoi could be removed so that the code would be concise。
After all, inside the code of ABACUS, the parameter of symmetry is treated as an integer. Besides that, treating the symmetry option as an integer is also similar to VASP and other codes, which would make it more user-friendly.

@maki49
Copy link
Collaborator Author

maki49 commented May 24, 2023

This is an excellent feature to let ABACUS decide how to set symmetry. But using a string type of symmetry parameter seems not necessary, you could provide a new integer option, e.g. symmetry=3, and use the following code to set symmetry to 0 or 1

    if (symmetry == 3)
    {   //deal with no-forced default value
        if (calculation == "nscf" || calculation == "get_S" || calculation == "istate" || calculation == "ienvelope")
            symmetry = 0;     //if md or exx, symmetry will be force-set to 0 or -1 later
        else
            symmetry = 1;
    }

the usage of std::stoi could be removed so that the code would be concise。 After all, inside the code of ABACUS, the parameter of symmetry is treated as an integer. Besides that, treating the symmetry option as an integer is also similar to VASP and other codes, which would make it more user-friendly.

Thanks for your suggestion.

Currently, there are several parameters with the type of int or real(double) using a string-type initialization with a stoi or stod to support flexible-default like this (and most of them are exx related parameters) .
So setting it as another integer is not a common solution. But I agree that there might be a solution both common and user-friendly.

I think we should merge this PR anyhow (for 1-PR-1-feature principle).

Then I will reopen the issue #2508 and discuss with @PeizeLin for the common and user-friendly solution.

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.

Should we set the default value of symmetry to 1?

3 participants