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.

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

XspectraCrystalWorkChain: Enable Symmetry Data Inputs #1028

Merged

Conversation

PNOGillespie
Copy link
Contributor

Overview

In this PR we add an input namespace for the XspectraCrystalWorkChain which allows the user to define the spacegroup and equivalent sites data for the incoming structure, thus instructing the WorkChain to generate structures and run calculations for only the sites specified. The new symmetry_data input requires entries for the system's spacegroup number (Int) and equivalent sites data (Dict). The former must simply be a valid spacegroup number (1-230) while the latter must contain an entry for each site (in the format site_<index>_<element>), where the minimal data required are the kind_name, element, site_index, and multiplicity.

In order to not be too restrictive on the user and make it easier to use external packages to build input symmetry data, we limit input validation to simple checks which ensure that the required data are present and that the data provided won't cause an obvious crash. Thus beyond the minimum required information, any extra data provided in the entries of the equivalent_sites_data node will be kept in case they may be needed by the user outside of the WorkChain (e.g. the list of symmetrically equivalent sites, which the WorkChain doesn't need).

Changes

  • Adds the symmetry_data input namespace to XspectraCrystalWorkChain, which the WorkChain will use to generate structures and set the list of polarisation vectors to calculate.
  • Adds input validation steps for the symmetry data to check for required information and for entries which may cause a crash, though does not check for issues beyond this in order to maximise flexibility of use.
  • Fixes an oversight in get_xspectra_structures where the supercell entry was not returned to the outputs when external symmetry data were provided by the user.

@PNOGillespie
Copy link
Contributor Author

Hi @superstar54, not sure what the issue with the docs is. If you could take a look at it (or ask someone who would know more about it), that would be much appreciated.

As always, I'm available to make changes as requested.

@superstar54 superstar54 self-requested a review May 23, 2024 06:22
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Thanks @PNOGillespie for the PR. I added a few minor suggestions.

src/aiida_quantumespresso/workflows/xspectra/crystal.py Outdated Show resolved Hide resolved
src/aiida_quantumespresso/workflows/xspectra/crystal.py Outdated Show resolved Hide resolved
PNOGillespie added a commit to PNOGillespie/aiida-quantumespresso that referenced this pull request May 23, 2024
Changes requested for PR aiidateam#1028:

* Small refactor of input validation code to condense checks into fewer
  lines.
* Re-arranged checks to ensure that `site_index` validation occurs
  *after* `required_keys` are checked. Previous behaviour would not
indicate which entry/entries were missing the `site_index`
* Fixed a small formatting error in the error message for mismatch in
  absorbing elements.
@PNOGillespie
Copy link
Contributor Author

Hi @superstar54, thanks for the review. I've committed your suggestions, as well as a couple of other things which I noticed on taking a second look at the input validation steps.

let me know if you need anything else.

@superstar54 superstar54 self-requested a review May 24, 2024 08:11
@PNOGillespie
Copy link
Contributor Author

Hi @superstar54, let me know if there are any other changes that you think are necessary for this PR.

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Hi @PNOGillespie , please fix the failed test, and then we can merge.

@PNOGillespie
Copy link
Contributor Author

Hi @PNOGillespie , please fix the failed test, and then we can merge.

Hi @superstar54. Looking at the output from the failed test, it seems to be a more general issue with the GitHub workflow - I see exactly the same thing in the recent PR by @yakutovicha (#1029).

Unfortunately, I don't know what the problem is. When I compile the docs in my docker container, everything runs fine, but here it returns a few errors:

  • WARNING: toctree contains reference to nonexisting document 'home/docs/checkouts/readthedocs.org/user_builds/aiida-quantumespresso/checkouts/1028/docs/source/reference/api/auto/aiida_quantumespresso/ - repeated for each entry in the API
  • /home/docs/checkouts/readthedocs.org/user_builds/aiida-quantumespresso/checkouts/1028/docs/source/reference/api/auto/aiida_quantumespresso/calculations/cp/index.rst:26: WARNING: py:obj reference target not found: base.CalcJob
  • /home/docs/checkouts/readthedocs.org/user_builds/aiida-quantumespresso/checkouts/1028/docs/source/reference/api/auto/aiida_quantumespresso/parsers/parse_xml/pw/parse/index.rst:71: WARNING: more than one target found for cross-reference 'XMLUnsupportedFormatError': aiida_quantumespresso.parsers.neb.XMLUnsupportedFormatError, aiida_quantumespresso.parsers.parse_xml.exceptions.XMLUnsupportedFormatError, aiida_quantumespresso.parsers.parse_xml.versions.XMLUnsupportedFormatError

Do we know anyone who might be able to diagnose and fix the problem?

@superstar54
Copy link
Member

Hi @mbercx , Could you please help check why the docs failed?

@superstar54 superstar54 self-requested a review July 10, 2024 11:22
superstar54
superstar54 previously approved these changes Jul 10, 2024
Comment on lines 181 to 188
help=(
'Input namespace to define equivalent sites and spacegroup number for the system. If defined, will '
+ 'skip symmetry analysis and structure standardization. Use *only* if symmetry data are known'
+ 'for certain. Requires ``spacegroup_number`` (Int) and ``equivalent_sites_data`` (Dict) to be'
+ 'defined separately. All keys in `equivalent_sites_data` must be formatted as "site_<site_index>".'
+ 'See docstring of `get_xspectra_structures` for more information about inputs.'
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure the + signs are superfluous since all strings are encapsulated by ()

Suggested change
help=(
'Input namespace to define equivalent sites and spacegroup number for the system. If defined, will '
+ 'skip symmetry analysis and structure standardization. Use *only* if symmetry data are known'
+ 'for certain. Requires ``spacegroup_number`` (Int) and ``equivalent_sites_data`` (Dict) to be'
+ 'defined separately. All keys in `equivalent_sites_data` must be formatted as "site_<site_index>".'
+ 'See docstring of `get_xspectra_structures` for more information about inputs.'
)
)
help=(
'Input namespace to define equivalent sites and spacegroup number for the system. If defined, will '
'skip symmetry analysis and structure standardization. Use *only* if symmetry data are known '
'for certain. Requires ``spacegroup_number`` (Int) and ``equivalent_sites_data`` (Dict) to be '
'defined separately. All keys in `equivalent_sites_data` must be formatted as "site_<site_index>". '
'See docstring of `get_xspectra_structures` for more information about inputs.'
)
)

raise ValidationError(
f'Input spacegroup number ({spacegroup_number}) outside of valid range (1-230).'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that the rest of the validator also raises exceptions, but the proper usage of a port validator is to return a string in case of a problem. So all raise ValidationError should simply be replaced by return.

PNOGillespie added a commit to PNOGillespie/aiida-quantumespresso that referenced this pull request Jul 10, 2024
Changes requested for PR aiidateam#1028 (2)

Corrects error handling behaviour for `validate_inputs` to use `return`
rather than `raise`.

Also fixes minor formatting errors in `symmetry_data` `help` entry and
removes unnecessary uses of `+`.
@PNOGillespie
Copy link
Contributor Author

PNOGillespie commented Jul 10, 2024

Hi @superstar54 and @sphuber, thanks to both of you for the feedback. I've added @sphuber's proposed changes (as well as fixing a little formatting whoopsy that I spotted in the help string that you were pointing to).

Let me know if any other changes are needed.

PNOGillespie and others added 4 commits July 11, 2024 09:18
Adds an input namespace for the `XspectraCrystalWorkChain` which
allows the user to define the spacegroup and equivalent sites data
for the incoming structure, thus instructing the WorkChain to generate
structures and run calculations for only the sites specified.

Changes:
* Adds the `symmetry_data` input namespace to `XspectraCrystalWorkChain`,
  which the `WorkChain` will use to generate structures and set the list
of polarisation vectors to calculate.
* Adds input validation steps for the symmetry data to check for
  required information and for entries which may cause a crash, though
does not check for issues beyond this in order to maximise flexibility
of use.
* Fixes an oversight in `get_xspectra_structures` where the `supercell`
  entry was not returned to the outputs when external symmetry data were
provided by the user.
Changes requested for PR aiidateam#1028:

* Small refactor of input validation code to condense checks into fewer
  lines.
* Re-arranged checks to ensure that `site_index` validation occurs
  *after* `required_keys` are checked. Previous behaviour would not
indicate which entry/entries were missing the `site_index`
* Fixed a small formatting error in the error message for mismatch in
  absorbing elements.
Changes requested for PR aiidateam#1028 (2)

Corrects error handling behaviour for `validate_inputs` to use `return`
rather than `raise`.

Also fixes minor formatting errors in `symmetry_data` `help` entry and
removes unnecessary uses of `+`.
@sphuber sphuber force-pushed the feature/xspectra/xs_crystal_sym_inputs branch from e741275 to 28d10f5 Compare July 11, 2024 07:18
@sphuber sphuber merged commit b79189d into aiidateam:main Jul 11, 2024
7 checks passed
@sphuber
Copy link
Contributor

sphuber commented Jul 11, 2024

Thanks @PNOGillespie

mikibonacci pushed a commit to mikibonacci/aiida-quantumespresso that referenced this pull request Aug 22, 2024
Adds an input namespace for the `XspectraCrystalWorkChain` which
allows the user to define the spacegroup and equivalent sites data
for the incoming structure, thus instructing the WorkChain to generate
structures and run calculations for only the sites specified.

Changes:
* Adds the `symmetry_data` input namespace to `XspectraCrystalWorkChain`,
  which the `WorkChain` will use to generate structures and set the list
  of polarisation vectors to calculate.
* Adds input validation steps for the symmetry data to check for
  required information and for entries which may cause a crash, though
  does not check for issues beyond this in order to maximise flexibility
  of use.
* Fixes an oversight in `get_xspectra_structures` where the `supercell`
  entry was not returned to the outputs when external symmetry data were
  provided by the user.
t-reents pushed a commit to t-reents/aiida-quantumespresso that referenced this pull request Nov 19, 2024
Adds an input namespace for the `XspectraCrystalWorkChain` which
allows the user to define the spacegroup and equivalent sites data
for the incoming structure, thus instructing the WorkChain to generate
structures and run calculations for only the sites specified.

Changes:
* Adds the `symmetry_data` input namespace to `XspectraCrystalWorkChain`,
  which the `WorkChain` will use to generate structures and set the list
  of polarisation vectors to calculate.
* Adds input validation steps for the symmetry data to check for
  required information and for entries which may cause a crash, though
  does not check for issues beyond this in order to maximise flexibility
  of use.
* Fixes an oversight in `get_xspectra_structures` where the `supercell`
  entry was not returned to the outputs when external symmetry data were
  provided by the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants