XAS: Fix Conflict Between tot_charge Override and CH Protocol
#809
Conversation
Addresses aiidalab#807. Fixes a conflict between `tot_charge` being set explicitly in overrides by default and the XAS plugin assuming that `tot_charge` isn't specified in the overrides.
|
@superstar54: No action needed here to keep XPS calculations working, though I would recommend adding a warning for users (either in documentation or in UI) to not do XPS calculations with charged cells. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #809 +/- ##
==========================================
- Coverage 68.76% 68.71% -0.05%
==========================================
Files 49 49
Lines 4245 4248 +3
==========================================
Hits 2919 2919
- Misses 1326 1329 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if adv_parameters["pw"]["parameters"]["SYSTEM"].get("tot_charge") == 0: | ||
| adv_parameters["pw"]["parameters"]["SYSTEM"].pop("tot_charge") |
There was a problem hiding this comment.
I think the logic should be: if using FCH,
adv_parameters["pw"]["parameters"]["SYSTEM"]["tot_charge"] += 1There was a problem hiding this comment.
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:
- 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). - 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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
In addition to the documentation, if the tot_charge is not zero, we need to raise a warning in the XAS/XPS GUI.
There was a problem hiding this comment.
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.
Addresses #807.
Fixes a conflict between
tot_chargebeing set explicitly in overrides by default and the XAS plugin assuming thattot_chargeisn't specified in the overrides. This change should restore the previous intended behaviour. For calculations of XAS or XPS with charged systems, it is recommended to instead warn the user that such calculations should be either avoided or carried out with care.