get_xspectra_structures: Refactor and Improve Code
#1026
Conversation
Refactors major components of `get_xspectra_structures` as part of ongoing improvement work. These changes are the first of two improvements intended to enable users to set symmetry data manually instead of relying on automatic symmetry analysis - ultimately enabling the same feature for the `XspectraCrystalWorkChain` and any others which use the same structure preparation tools. Changes: * Moves supercell creation, processes for molecules, and generation of `equivalent_sites_data` to separate functions. * Adds functionality for users to manually provide symmetry data, thus enabling the `CalcFunction` to be used as a means to generate structures with user control over which exact sites to mark. * Fixes a bug discovered in the case where non-hubbard structures with custom Kind names lost their Kind names when converting the ASE supercell to `StructureData` type. * Fixes a small oversight in the tests where `spglib_settings` was mistakenly named `spglib_options`.
There was a problem hiding this comment.
Thanks, @PNOGillespie, for the work!
Could you also add a test when setting symmetry data manually.
| from pymatgen.symmetry.analyzer import PointGroupAnalyzer | ||
|
|
||
| result = {'output_params': {}} | ||
| # If we are working with a molecule, check for pymatgen_settings |
There was a problem hiding this comment.
| # If we are working with a molecule, check for pymatgen_settings |
There was a problem hiding this comment.
Could you also fix this? The structure is a molecule inside this function.
There was a problem hiding this comment.
Sorry about that. I thought I'd removed the comment in the first set of corrections that I committed. It's been removed in the latest commit
| for the input structure. If False, will instead use symmetry data provided | ||
| via the `equivalent_sites_data` and `spacegroup_number` inputs and generate all | ||
| the structures based on this information. Defaults to True. | ||
| - equivalent_sites_data: a Dict object taking the form of the `equivalent_sites_data` dict normally |
There was a problem hiding this comment.
The description of equivalent_sites_data is not clear to the user.
| is_molecule_input = False | ||
| if 'is_molecule_input' in unwrapped_kwargs: | ||
| is_molecule_input = kwargs['is_molecule_input'].value |
There was a problem hiding this comment.
| is_molecule_input = False | |
| if 'is_molecule_input' in unwrapped_kwargs: | |
| is_molecule_input = kwargs['is_molecule_input'].value | |
| is_molecule_input = kwargs.get('is_molecule_input', False) |
this should work because one can use AiiDA Bool data in the if condition directly.
1st set of changes requested for aiidateam#1026: * Added test of manual symmetry data input functionality. * Removed unnecessary checks for keys of Bool nodes in `unwrapped_kwargs` as well as extracting of truth values in favour of using Bool objects directly. * Removed unnecessary comment in `process_molecular_system` * Added example to docstring to demonstrate format for `equivalent_sites_data` input
|
Hi @superstar54, Thanks for the feedback. I've added a simple test for manual symmetry data input and cleaned up a few things as you suggested doing in one of your comments. I also added an example of how the |
Changes the docstring so that the `equivalent_sites_data` example is in-line, as required by the docs. Also adds a sentence stating which options are required and that all other options are ignored.
|
Hi @superstar54. I fixed the issue with the docstring by formatting the example to be in-line. I also added a sentence to better explain what needs to be in the Let me know if anything else is needed. |
Removes an unnecessary comment from `process_molecule_input` which should have been removed in a previous commit.
Overview
In this PR, we have re-factored various components of
get_xspectra_structuresas part of ongoing improvement work. These changes are the first of two improvements intended to enable users to set symmetry data manually instead of relying on automatic symmetry analysis - ultimately enabling the same feature for theXspectraCrystalWorkChainand any others which use the same structure preparation tools.This PR will be followed by a second one in which the logic for the XSpectra
WorkChains will be added in order to properly exploit the changes in this PR. The overall changes have been separated into 2 PRs due to the number of changes made here as part of the re-factoring process.Changes
equivalent_sites_datato separate functions.CalcFunctionto be used as a means to generate structures with user control over which exact sites to mark.StructureDatatype.dict.keys()iterables where simply iterating over the dictionary directly can be used instead.spglib_settingswas mistakenly namedspglib_options.