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

PNOGillespie · 2023-09-21T10:12:43Z

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

get_xspectra_structures:
- Passes Hubbard parameters from the input structure to the structures returned by the CalcFunction. If a supercell is created from the input structure, then the parameters are scaled to the new supercell using HubbardUtils.get_hubbard_for_supercell().
- Fixes a previous issue with the CalcFunction where the information on equivalent atomic sites was taken from the standardized structure even if the input structure was not standardized.
- Introduces a new Bool input use_element_types which instructs the CalcFunction to treat Kinds of the same element as the same for the purposes of symmetry analysis even if their kind_names differ.
XspectraCrystalWorkChain:
- Updates the WorkChain to set standardize_structure = False if the input structure is HubbardStructureData in order to preserve the Hubbard data during structure creation.
- Automatically sets the starting_magnetization of the absorbing atom in each subsequent XspectraCoreWorkChain based on the magnetic moment given for the Kind that it replaces. Note that this only works if starting_magnetization is provided as a dict instead of separate entries of starting_magnetization(n).
XspectraCoreWorkChain:
- Updates the WorkChain so that it applies the GIPAW pseudopotential provided for the calculation to all Kinds corresponding to the absorbing element even if multiple of such exist.

…get_xspectra_structures` This commit enables `XspectraCoreWorkChain` and `get_xspectra_structures` to correctly parse `HubbardStructureData` and magnetic moments. Currently enabled: * `get_xspectra_structures` will preserve Hubbard data if the input structure is `HubbardStructureData` node type and scale parameters to match those required for the final supercell. * the `get_builder_from_protocol` function of `XspectraCoreWorkChain` will accept `initial_magnetic_moments` as an optional input and pass this information to the `scf` namespace. To do: * Enable `get_xspectra_structures` to process incoming magnetic moments data and return correct magnetic moments for the structures returned by the function. * Update the `run_upf2plotcore` step of `XspectraCoreWorkChain` to reliably pick the correct pseudo for the ShellJob step when more than one Kind of the absorbing element is present. * Implement steps in the `XspectraCrystalWorkChain` to correctly parse both HubbardStructureData and magnetic moments.

Updates `get_xspectra_structures()` to retrieve and re-apply Kind data to output structures so that Kind names are kept. The function will now also correctly determine the list of inequivalent atom sites if `standardize_structure = False` and return the correct set of marked structures.

Removes options for `initial_magnetization` from the function, since this has been shown to be superfluous in practice.

This commit fully enables the `XspectraCrystalWorkChain` to work with `HubbardStructureData` and to apply `initial_magnetic_moments` to all sub-processes. Automatically sets `standardize_structure` to `False` in the `get_xspectra_structures` step of the `WorkChain` if the input structure is `HubbardStructureData` as required by the `CalcFunction`. Also updates `get_xspectra_structures` with a new optional parameter `use_element_types` which instructs the `CalcFunction` to treat all `Kinds` of the same element as the same for the purposes of symmetry analysis even if the `kind_name`s are different. Defaults to `False`.

Updates the `run_all_xspectra_core` step in the `XspectraCrystalWorkChain` to correctly assign the GIPAW pseudo provided for the element to all `Kind`s corresponding to the element, retaining the step to remove the GIPAW pseudo if there is only one atom of the absorbing element present in the structure (and thus, to avoid a crash due to incorrect pseudo allocation).

superstar54

Hi @PNOGillespie , thanks for the work.
Could you add a test for the get_xspectra_structure using HubbardStructureData?

src/aiida_quantumespresso/workflows/functions/get_xspectra_structures.py

superstar54 · 2023-11-22T11:34:28Z

src/aiida_quantumespresso/workflows/functions/get_xspectra_structures.py

+            for i in spglib_tuple[2]:
+                if i >= 1000:
+                    new_i = int(i / 1000)
+                else:
+                    new_i = i
+                clean_structure_tuple[2].append(new_i)


Could you add a comment to explain the codes?

There should now be a more detailed comment above this block of code to explain what's going on, let me know if this is clear enough.

Addresses requested changes in PR aiidateam#969 and refines the logic for assigning magnetic states to the absorbing atom depending on chosen core-hole treatment type, as some oversights were made in the case of magnetic systems. The absorbing atom is now always given a spin of 1 if it was 0 in the neutral system and we are using an XCH-type treatment. Otherwise, the spin is set to the value of its inherited `Kind`. Other refinements: * Fixed a typo where the core-hole treatments would override the overrides dictionary (the opposite to intended behaviour). * Removed unnecessary options for SpinType and initial_magnetic_moments in the `CoreWorkChain`. * Added tests for `get_xspectra_structures` to cover basic behaviour and correct handling of HubbardStructureData.

PNOGillespie · 2023-11-29T10:58:19Z

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.

Changes: * Fixes a bug where, if `use_element_types = True`, the symmetry data of the "non-cleaned" structure would be erroneously returned in `output_parameters` * Fixes a bug where, if `use_element_types = True`, the `kind_name` for each entry in `equivalent_sites_data` would be reported incorrectly. * Changes the default setting of `use_element_types` to `True`. * Changes the logic for converting the atomic number of each `type` in `spglib_tuple` used to generate the `cleaned_structure_tuple` to use `np.trunc(int()) instead of just `int()`.

…Gillespie/aiida-quantumespresso into fix/xs-crystal-wc-structures

Updates `test_use_element_types` following changes to default function behaviour introduced in previous commit (c4701b3)

PNOGillespie · 2024-03-04T10:45:36Z

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.

superstar54

@PNOGillespie Thanks for the update!
I have a feeling that the get_xspectra_structures function is too long and complex with many nested for and if block. Thus, it's hard for people to follow the logic. I would suggest simplifying its logic through modularization. You can create sub-functions for some key tasks, such as the generate_hubbard_supercell function, etc.

superstar54 · 2024-03-19T11:39:09Z

src/aiida_quantumespresso/workflows/functions/get_xspectra_structures.py

+        if use_element_types:
+            cleaned_structure_tuple = (spglib_tuple[0], spglib_tuple[1], [])
+            for i in spglib_tuple[2]:
+                if i >= 1000:
+                    new_i = int(np.trunc(i / 1000))
+                else:
+                    new_i = i
+                cleaned_structure_tuple[2].append(new_i)
+            cleaned_symmetry_dataset = spglib.get_symmetry_dataset(cleaned_structure_tuple, **spglib_kwargs)


I think there is no need to create a new clearned_symmetry_dataset here. For the following reasons:

Since the symmetry_dataset is used to check if standardization is needed or not. use_element_types will affect the check.

There are too many if use_element_types: check, we can avoid it if use the same name.

Suggested change

if use_element_types:

cleaned_structure_tuple = (spglib_tuple[0], spglib_tuple[1], [])

for i in spglib_tuple[2]:

if i >= 1000:

new_i = int(np.trunc(i / 1000))

else:

new_i = i

cleaned_structure_tuple[2].append(new_i)

cleaned_symmetry_dataset = spglib.get_symmetry_dataset(cleaned_structure_tuple, **spglib_kwargs)

if use_element_types:

cleaned_structure_tuple = (spglib_tuple[0], spglib_tuple[1], [])

for i in spglib_tuple[2]:

if i >= 1000:

new_i = int(np.trunc(i / 1000))

else:

new_i = i

cleaned_structure_tuple[2].append(new_i)

symmetry_dataset = spglib.get_symmetry_dataset(cleaned_structure_tuple, **spglib_kwargs)

else:

symmetry_dataset = spglib.get_symmetry_dataset(spglib_tuple, **spglib_kwargs)

Removes various unneeded `if` statements and re-uses information generated during the process flow to simplify steps.

PNOGillespie · 2024-03-22T11:29:53Z

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.

superstar54

A small change is needed.

superstar54 · 2024-03-28T14:12:57Z

src/aiida_quantumespresso/workflows/functions/get_xspectra_structures.py

+            spglib_std_types = spglib.get_symmetry_dataset(spglib_tuple, **spglib_kwargs)['std_types']
+            spglib_map_to_prim = spglib.get_symmetry_dataset(spglib_tuple, **spglib_kwargs)['mapping_to_primitive']
+            spglib_std_map_to_prim = spglib.get_symmetry_dataset(spglib_tuple,
+                                                                 **spglib_kwargs)['std_mapping_to_primitive']


please change it to run get_symmetry_dataset only one time.

Changes requested for PR aiidateam#969 Replaces some uses of `spglib.get_symmetry_dataset` with a single function call.

PNOGillespie · 2024-03-28T15:12:50Z

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

superstar54

LGTM! Thanks for the work! @PNOGillespie

PNOGillespie and others added 7 commits August 30, 2023 12:50

Tests: Fix minor errors in tests from previous commit

1d17eaa

get_xspectra_structures(): remove options for initial_magnetization

d685dcd

Removes options for `initial_magnetization` from the function, since this has been shown to be superfluous in practice.

Merge branch 'aiidateam:main' into fix/xs-crystal-wc-structures

96ff74f

superstar54 self-requested a review November 9, 2023 12:39

Merge branch 'main' into fix/xs-crystal-wc-structures

9ae5570

superstar54 requested changes Nov 22, 2023

View reviewed changes

PNOGillespie and others added 2 commits November 29, 2023 11:26

Merge branch 'main' into fix/xs-crystal-wc-structures

8b71067

PNOGillespie requested a review from superstar54 November 29, 2023 10:58

PNOGillespie mentioned this pull request Dec 13, 2023

Add Feature: XAS Plugin aiidalab/aiidalab-qe#580

Merged

PNOGillespie mentioned this pull request Feb 2, 2024

Example structure SiO2 has extra tags aiidalab/aiidalab-qe#617

Closed

PNOGillespie and others added 4 commits March 4, 2024 09:55

Merge branch 'main' into fix/xs-crystal-wc-structures

120ae6d

Merge branch 'fix/xs-crystal-wc-structures' of https://github.com/PNO…

24524e5

…Gillespie/aiida-quantumespresso into fix/xs-crystal-wc-structures

Tests: Update test_get_xspectra_structures

1f1d8f5

Updates `test_use_element_types` following changes to default function behaviour introduced in previous commit (c4701b3)

superstar54 requested changes Mar 19, 2024

View reviewed changes

get_xspectra_structures: Consolidate Logic

7f3842f

Removes various unneeded `if` statements and re-uses information generated during the process flow to simplify steps.

superstar54 self-requested a review March 28, 2024 14:18

superstar54 requested changes Mar 28, 2024

View reviewed changes

get_xspectra_structures: Reduce Use of get_symmetry_dataset

d34f16f

Changes requested for PR aiidateam#969 Replaces some uses of `spglib.get_symmetry_dataset` with a single function call.

superstar54 self-requested a review March 28, 2024 15:18

superstar54 approved these changes Mar 28, 2024

View reviewed changes

Merge branch 'main' into fix/xs-crystal-wc-structures

24695da

superstar54 merged commit f439504 into aiidateam:main Mar 29, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PNOGillespie commented Sep 21, 2023

superstar54 left a comment

superstar54 Nov 22, 2023

PNOGillespie Nov 29, 2023

PNOGillespie commented Nov 29, 2023

PNOGillespie commented Mar 4, 2024

superstar54 left a comment

superstar54 Mar 19, 2024

PNOGillespie commented Mar 22, 2024

superstar54 left a comment

superstar54 Mar 28, 2024

PNOGillespie commented Mar 28, 2024

superstar54 left a comment

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

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

Conversation

PNOGillespie commented Sep 21, 2023

Overview

Changes

superstar54 left a comment

Choose a reason for hiding this comment

superstar54 Nov 22, 2023

Choose a reason for hiding this comment

PNOGillespie Nov 29, 2023

Choose a reason for hiding this comment

PNOGillespie commented Nov 29, 2023

PNOGillespie commented Mar 4, 2024

superstar54 left a comment

Choose a reason for hiding this comment

superstar54 Mar 19, 2024

Choose a reason for hiding this comment

PNOGillespie commented Mar 22, 2024

superstar54 left a comment

Choose a reason for hiding this comment

superstar54 Mar 28, 2024

Choose a reason for hiding this comment

PNOGillespie commented Mar 28, 2024

superstar54 left a comment

Choose a reason for hiding this comment