aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
55 stars 82 forks source link

XAS: Enable Correct Parsing of Hubbard and Magnetic Data #969

Closed PNOGillespie closed 8 months ago

PNOGillespie commented 1 year ago

Overview

In this PR, we update the various WorkChains and CalcFunctions relevant for calculations with XSpectra to work with the new HubbardStructureData data type and to handle system with magnetic data reliably, thus making calculations of systems with known magnetic and Hubbard parameter configurations possible with each WorkChain.

Changes

PNOGillespie commented 1 year ago

Hi @superstar54, I have added some tests for get_xspectra_structures and also refined some of the logic for assigning magnetic states, since I noticed that the original method would not give the absorbing atom the correct starting_magnetization if the Kind that it inherited from had a starting_magnetization of zero. I also fixed a few mistakes that I noticed when I testing it again.

PNOGillespie commented 9 months ago

Hi @superstar54. I noted (and fixed) a bug that I spotted with the function which will be relevant for cases where different Kind names applied to the structure would break the symmetry (e.g. in the case of antiferromagnetic systems). This should now be ready for final review.

If you could give the PR another review (or ping someone with write access) that would be much appreciated.

PNOGillespie commented 8 months ago

Hi @superstar54. I reviewed the source code to see if I could de-convolute and consolidate some of the process logic. I have managed to reduce the number of if statements somewhat in a few different ways, but modularisation won't simplify things here since there are very few cases where code is really re-used.

In regards to your suggestion here:

think there is no need to create a new clearned_symmetry_dataset here.

The CalcFunction needs to know which Kind should be present at each site, so both "cleaned" and "non-cleaned" datasets are needed in order to map the correct Kind to each chosen absorbing atom while also determining the symmetry properties correctly (i.e. where we assume that all Kinds of the same element are the same). This information is then needed by XspectraCrystalWorkChain in order to correctly set the magnetic moment (particularly for an anti-ferromagnetic system).

I reduced the number of if use_element_types statements by taking your suggestion and then generating the arrays needed to determine which Kinds correspond to which site on-the-fly, since the alternative is to constantly check if use_element_types == True.

All this behaviour should be covered by the tests, since there are test cases for c-Si using [Si, Si] and [Si0, Si1]

Let me know what you think of the changes.

PNOGillespie commented 8 months ago

Hi @superstar54, I've changed it to now generate a single dataset and extract the data from there instead of repeating the same function call.

Let me know if you need anything else