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.

Sign up for GitHub

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

XAS: Fix Conflict Between tot_charge Override and CH Protocol #809

Merged
merged 5 commits into from
Oct 31, 2024
Merged

XAS: Fix Conflict Between tot_charge Override and CH Protocol #809

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
04d2e90
XAS: Fix Conflict Between `tot_charge` Override and CH Protocol
PNOGillespie Sep 24, 2024
1e698c3
Merge branch 'main' into fix/xas/tot_charge
PNOGillespie Oct 25, 2024
04e6ff4
XAS: Add Documentation Warning for Charged Systems
PNOGillespie Oct 25, 2024
86f6a6a
Merge branch 'main' into fix/xas/tot_charge
superstar54 Oct 28, 2024
7e57056
Merge branch 'main' into fix/xas/tot_charge
superstar54 Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/aiidalab_qe/plugins/xas/workchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ def update_resources(builder, codes):
def get_builder(codes, structure, parameters, **kwargs):
from copy import deepcopy

adv_parameters = deepcopy(parameters["advanced"])
# Setting `tot_charge = 0` will cause FCH calculations to fail due to
# inputs being incorrect, thus we pop this from the overrides
if adv_parameters["pw"]["parameters"]["SYSTEM"].get("tot_charge") == 0:
adv_parameters["pw"]["parameters"]["SYSTEM"].pop("tot_charge")
Comment on lines +29 to +30
Copy link
Member

Choose a reason for hiding this comment

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

I think the logic should be: if using FCH,

adv_parameters["pw"]["parameters"]["SYSTEM"]["tot_charge"] += 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this was my thinking originally, on the basis that (nominally) FCH screens the core-hole by adding a +1 charge to the system, so it would be logical to simply do tot_charge += 1. However, there are two things to bear in mind:

  1. There aren't any cases in the literature for XAS calculations performed on already-charged systems. Everything else has been done on a system which starts out neutral (tot_charge = 0).
  2. It's safe to assume that if the user is working with the advanced options, then they should be able to play around with the options without the program interfering in a way which they cant see happening.

As said in the top comment: for charged systems, it would be better instead to let the user set things as they wish and advise them via the documentation (which I will update later when I get the time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to add: It's safe (IMO) to let users run XAS with FCH alongside other properties for neutral systems, since we've tested that extensively by now, but we should instead advise users to run XAS separately for charged systems and test the behaviour if they're using FCH. For XCH, it's probably fine, but should ideally be tested anyway

Copy link
Member

Choose a reason for hiding this comment

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

In addition to the documentation, if the tot_charge is not zero, we need to raise a warning in the XAS/XPS GUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I will work on adding a warning to the GUI for XAS. In principle this should only apply to FCH XAS and molecule XPS calculations. Anything to do with XCH (XAS or XPS) should not require a serious warning, but adding a section to the documentation for XAS and XPS and pointing to the section would be the ideal solution IMO.

protocol = parameters["workchain"]["protocol"]
xas_parameters = parameters["xas"]
core_hole_treatments = xas_parameters["core_hole_treatments"]
Expand Down Expand Up @@ -61,7 +66,7 @@ def get_builder(codes, structure, parameters, **kwargs):
xs_code = codes["xspectra"]["code"]
overrides = {
"core": {
"scf": deepcopy(parameters["advanced"]),
"scf": adv_parameters,
# PG: Here, we set a "variable" broadening scheme, which actually defines a constant broadening
# The reason for this is that in "gamma_mode = constant", the Lorenzian broadening parameter
# is defined by "xgamma" (in "PLOT"), but this parameter *also* controls the broadening value
Expand Down
Loading