From e22b5e70a16e8df82332b4cb82727496eb54e3c4 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Sun, 9 Oct 2022 17:45:46 +0200 Subject: [PATCH] CLI: block family cutoffs set for established families One of the motivations of having dedicated and automated CLI commands to install established pseudpotential families like the SSSP or the Pseudo-dojo normconserving pseudopotentials is to try and prevent that the pseudopotentials or recommended cutoffs deviate from the official ones. However, the `family cutoffs set` command currently allows the user to change the recommended cutoffs easily through the CLI. In order to prevent this, we block usage of the command for `SsspFamily` and `PseudoDojoFamily`'s, by adding an `exclude` input argument to the `PseudoPotentialFamilyParam`, where the entry points of the classes that should be disallowed can be provided. This is then used to adapt the option decorator for the `PSEUDO_POTENTIAL_FAMILY` input argument of `family cutoffs set`. Note that the user _can_ still install e.g. the SSSP with their own recommended cutoffs using the `install family` command as a `CutoffsPseudoPotentialFamily`. However, the `SsspFamily` class is reserved for pseudopotentials installed with the automated install command. --- docs/source/conf.py | 7 ++++- docs/source/design.rst | 40 ++++++++++++++++--------- src/aiida_pseudo/cli/family.py | 6 ++-- src/aiida_pseudo/cli/params/types.py | 45 +++++++++++++++++++++------- tests/cli/test_family.py | 27 +++++++++++++++++ 5 files changed, 97 insertions(+), 28 deletions(-) diff --git a/docs/source/conf.py b/docs/source/conf.py index 0eacdec..0fb69f2 100644 --- a/docs/source/conf.py +++ b/docs/source/conf.py @@ -20,7 +20,7 @@ # Add any Sphinx extension module names here, as strings. They can be # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom # ones. -extensions = ['sphinx_copybutton', 'autoapi.extension', 'sphinx_click'] +extensions = ['sphinx_copybutton', 'autoapi.extension', 'sphinx_click', 'sphinx.ext.intersphinx'] # Settings for the `sphinx_copybutton` extension copybutton_selector = 'div:not(.no-copy)>div.highlight pre' @@ -31,6 +31,11 @@ autoapi_dirs = ['../../src/aiida_pseudo'] autoapi_ignore = ['*cli*'] +# Settings for the `sphinx.ext.intersphinx` extension +intersphinx_mapping = { + 'aiida': ('http://aiida_core.readthedocs.io/en/latest/', None), +} + # Add any paths that contain templates here, relative to this directory. templates_path = ['_templates'] diff --git a/docs/source/design.rst b/docs/source/design.rst index eb79553..dacd21e 100644 --- a/docs/source/design.rst +++ b/docs/source/design.rst @@ -11,14 +11,14 @@ The two concepts are further explained below, especially focusing on how they ar Pseudopotentials ================ -Pseudopotentials are implemented as `data plugins `_, and the base class is ``PseudoPotentialData``. +Pseudopotentials are implemented as `data plugins `_, and the base class is :py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData`. Pseudopotentials are assumed to be defined by a single file on disk and represent a single chemical element. As such, each pseudopotential node, *has* to have two attributes: the md5 of the file that it represents and the symbol of chemical element that the pseudopotential describes. -The latter follows IUPAC naming conventions as used in the module ``aiida.common.constants.elements`` of ``aiida-core``. +The latter follows IUPAC naming conventions defined as ``elements`` in the module ``aiida.common.constants`` of ``aiida-core``. -The ``PseudoPotentialData`` functions as a base class and does not represent an actual existing pseudopotential format. -It *is* possible to create a family of ``PseudoPotentialData`` nodes, but since the element cannot be parsed from the file content itself, it will have to be done from the *filename*, so the filename *has* to have the format ``CHEMICAL_SYMBOL.EXTENSION``, for example ``Ar.pseudo``. -Since the ``PseudoPotentialData`` class does not have any features tailored to specific formats, as do the classes which inherit from it (e.g. ``UpfData`` for UPF-formatted pseudopotentials), this class is mostly useful for development. +:py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData` functions as a base class and does not represent an actual existing pseudopotential format. +It *is* possible to create a family of :py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData` nodes, but since the element cannot be parsed from the file content itself, it will have to be done from the *filename*, so the filename *has* to have the format ``CHEMICAL_SYMBOL.EXTENSION``, for example ``Ar.pseudo``. +Since the :py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData` class does not have any features tailored to specific formats, as do the classes which inherit from it (e.g. :py:class:`~aiida_pseudo.data.pseudo.UpfData` for UPF-formatted pseudopotentials), this class is mostly useful for development. The plugin comes with a variety of pseudopotential subtypes that represent various common pseudopotential formats, such as UPF, PSF, PSML and PSP8. The corresponding data plugins implement how the element and other data are parsed from a pseudopotential file with such a format. @@ -52,11 +52,11 @@ Note that if more than one pseudopotential in the database is matched, a random Families ======== -Having loose pseudopotentials floating around is not very practical, so groups of related pseudopotentials can be organized into "families", which are implemented as group plugins with the base class ``PseudoPotentialFamily``. +Having loose pseudopotentials floating around is not very practical, so groups of related pseudopotentials can be organized into "families", which are implemented as group plugins with the base class :py:class:`~aiida_pseudo.groups.family.PseudoPotentialFamily`. A family class can in principle support many pseudopotential formats, however, a family instance can only contain pseudopotentials of a single format. -For example, the ``PseudoPotentialFamily`` class supports all of the pseudopotential formats that are supported by this plugin. +For example, the :py:class:`~aiida_pseudo.groups.family.PseudoPotentialFamily` class supports all of the pseudopotential formats that are supported by this plugin. However, any instance can only contain pseudopotentials of the same format (e.g. *all* UPF or *all* PSP8, not a mixture). -In contrast, the ``SsspFamily`` only supports the ``UpfData`` format. +In contrast, the :py:class:`~aiida_pseudo.groups.family.SsspFamily` only supports the :py:class:`~aiida_pseudo.data.pseudo.UpfData` format. A pseudopotential family can be constructed manually, by first constructing the class instance and then adding pseudopotential data nodes to it: @@ -77,7 +77,7 @@ A pseudopotential family can be constructed manually, by first constructing the family = PseudoPotentialFamily(label='pseudos/upf').store() family.append(pseudos) -Note that as with any ``Group``, it has to be stored before nodes can be added. +Note that as with any :py:class:`~aiida.orm.Group`, it has to be stored before nodes can be added. If you have a folder on disk that contains various pseudopotentials for different elements, there is an even easier way to create the family automatically: .. code-block:: python @@ -90,17 +90,29 @@ If you have a folder on disk that contains various pseudopotentials for differen family = PseudoPotentialFamily('path/to/pseudos', 'pseudos/upf', pseudo_type=UpfData) The plugin is not able to reliably deduce the format of the pseudopotentials contained in the folder, so one should indicate what data type to use with the ``pseudo_type`` argument. -The exception is when the family class only supports a single pseudo type, such as for the ``SsspFamily``, in which case that type will automatically be selected. +The exception is when the family class only supports a single pseudo type, such as for the :py:class:`~aiida_pseudo.groups.family.SsspFamily`, in which case that type will automatically be selected. Subclasses of supported pseudo types are also accepted. -For example, the base class ``PseudoPotentialFamily`` supports pseudopotentials of the ``PseudoPotentialData`` type. -Because all more specific pseudopotential types are subclasses of ``PseudoPotentialData``, the ``PseudoPotentialFamily`` class accepts all of them. +For example, the base class :py:class:`~aiida_pseudo.groups.family.PseudoPotentialFamily` supports pseudopotentials of the :py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData` type. +Because all more specific pseudopotential types are subclasses of py:class:`~aiida_pseudo.data.pseudo.PseudoPotentialData`, the :py:class:`~aiida_pseudo.groups.family.PseudoPotentialFamily` class accepts all of them. + +Established families +-------------------- + +When it comes to pseudopotential families, ``aiida-pseudo`` makes a clear distinction between families that are *established* and those that are not. +A pseudopotential family is only considered to be *established* when it has a comprehensive set of rigorously tested pseudopotentials with convergence tests, both of which have been published and are publicly available. + +Only a pseudopotential family that is *established* will receive support for automated installs with its own class (e.g. :py:class:`~aiida_pseudo.groups.family.SsspFamily`/:py:class:`~aiida_pseudo.groups.family.PseudoDojoFamily`) and command line interface (CLI) commands (e.g. ``install sssp``/``install pseudo-dojo``). +To make sure these families represent the official ones, they can only be installed with their supported CLI commands, and there are strict checks on the format of these files to make sure they correspond to the official ones. +Based on the same principle of preserving the integrity of these established pseudopotentials, the ``family cutoffs set`` command can not be used to set the recommended cutoffs of an established family. + +If users want to install the a set of *non-established* pseudopotentials and configure their recommended cutoffs, they have to install them from the archive using ``install family`` as a :py:class:`~aiida_pseudo.groups.family.CutoffsPseudoPotentialFamily` as described in the :ref:`corresponding how-to section `. Recommended cutoffs =================== -Certain pseudopotential family types, such as the ``SsspFamily``, provide recommended cutoff values for wave functions and charge density in plane-wave codes. +Certain pseudopotential family types, such as the :py:class:`~aiida_pseudo.groups.family.SsspFamily`, provide recommended cutoff values for wave functions and charge density in plane-wave codes. These cutoffs can be defined in any unit supported by the |pint|_ package. -The recommended cutoffs for a set of elements or a ``StructureData`` can be retrieved from the family as follows: +The recommended cutoffs for a set of elements or a :py:class:`~aiida.orm.StructureData` can be retrieved from the family as follows: .. code-block:: python diff --git a/src/aiida_pseudo/cli/family.py b/src/aiida_pseudo/cli/family.py index 15ae363..bcda8e0 100644 --- a/src/aiida_pseudo/cli/family.py +++ b/src/aiida_pseudo/cli/family.py @@ -7,7 +7,7 @@ import click from ..groups.mixins import RecommendedCutoffMixin -from .params import arguments, options +from .params import arguments, options, types from .root import cmd_root @@ -56,7 +56,9 @@ def cmd_family_cutoffs(): @cmd_family_cutoffs.command('set') -@arguments.PSEUDO_POTENTIAL_FAMILY() +@arguments.PSEUDO_POTENTIAL_FAMILY( + type=types.PseudoPotentialFamilyParam(exclude=('pseudo.family.sssp', 'pseudo.family.pseudo_dojo')) +) @click.argument('cutoffs', type=click.File(mode='rb')) @options.STRINGENCY(required=True) @options.UNIT() diff --git a/src/aiida_pseudo/cli/params/types.py b/src/aiida_pseudo/cli/params/types.py index d1d64e4..50d3875 100644 --- a/src/aiida_pseudo/cli/params/types.py +++ b/src/aiida_pseudo/cli/params/types.py @@ -15,16 +15,17 @@ class PseudoPotentialTypeParam(click.ParamType): - """Parameter type for `click` commands to define a subclass of `PseudoPotentialData`.""" + """Parameter type for ``click`` commands to define a subclass of ``PseudoPotentialData``.""" name = 'pseudo_type' def convert(self, value, _, __): """Convert the entry point name to the corresponding class. - :param value: entry point name that should correspond to subclass of `PseudoPotentialData` data plugin - :return: the `PseudoPotentialData` subclass - :raises: `click.BadParameter` if the entry point cannot be loaded or is not subclass of `PseudoPotentialData` + :param value: entry point name that should correspond to subclass of ``PseudoPotentialData`` data plugin + :return: the ``PseudoPotentialData`` subclass + :raises: ``click.BadParameter`` if the entry point cannot be loaded or is not subclass of + ``PseudoPotentialData`` """ from aiida.common import exceptions from aiida.plugins import DataFactory @@ -54,13 +55,34 @@ def complete(self, _, incomplete): class PseudoPotentialFamilyParam(GroupParamType): - """Parameter type for `click` commands to define an instance of a `PseudoPotentialFamily`.""" + """Parameter type for ``click`` commands to define an instance of a ``PseudoPotentialFamily``.""" name = 'pseudo_family' + def __init__(self, exclude: typing.Optional[typing.List[str]] = None, **kwargs): + """Construct the parameter. + + :param exclude: an optional list of values that should be considered invalid and will raise ``BadParameter``. + """ + super().__init__(**kwargs) + self.exclude = exclude + + def convert(self, value, param, ctx): + """Convert the entry point name to the corresponding class. + + :param value: entry point name that should correspond to subclass of ``PseudoPotentialFamily`` group plugin + :return: the ``PseudoPotentialFamily`` subclass + :raises: `click.BadParameter` if the entry point cannot be loaded or is not subclass of `PseudoPotentialFamily` + """ + family = super().convert(value, param, ctx) + + if self.exclude and family.type_string in self.exclude: + self.fail(f'The value `{family}` is not allowed for this parameter.', param, ctx) + return family + class PseudoPotentialFamilyTypeParam(click.ParamType): - """Parameter type for `click` commands to define a subclass of `PseudoPotentialFamily`.""" + """Parameter type for ``click`` commands to define a subclass of ``PseudoPotentialFamily``.""" name = 'pseudo_family_type' @@ -75,9 +97,10 @@ def __init__(self, exclude: typing.Optional[typing.List[str]] = None, **kwargs): def convert(self, value, _, __): """Convert the entry point name to the corresponding class. - :param value: entry point name that should correspond to subclass of `PseudoPotentialFamily` group plugin - :return: the `PseudoPotentialFamily` subclass - :raises: `click.BadParameter` if the entry point cannot be loaded or is not subclass of `PseudoPotentialFamily` + :param value: entry point name that should correspond to subclass of ``PseudoPotentialFamily`` group plugin + :return: the ``PseudoPotentialFamily`` subclass + :raises: `click.BadParameter` if the entry point cannot be loaded or is not subclass of ` + `PseudoPotentialFamily`` """ from aiida.common import exceptions from aiida.plugins import GroupFactory @@ -134,7 +157,7 @@ def convert(self, value, param, ctx) -> typing.Union[pathlib.Path, bytes]: class UnitParamType(click.ParamType): - """Parameter type to specify units from the `pint` library.""" + """Parameter type to specify units from the ``pint`` library.""" name = 'unit' @@ -149,7 +172,7 @@ def __init__(self, quantity: typing.Optional[typing.List[str]] = None, **kwargs) def convert(self, value, _, __): """Check if the provided unit is a valid unit for the defined quantity. - :raises: `click.BadParameter` if the provided unit is not valid for the quantity defined for this instance. + :raises: ``click.BadParameter`` if the provided unit is not valid for the quantity defined for this instance. """ if value not in U: raise click.BadParameter(f'`{value}` is not a valid unit.') diff --git a/tests/cli/test_family.py b/tests/cli/test_family.py index 230e3bb..5e24adb 100644 --- a/tests/cli/test_family.py +++ b/tests/cli/test_family.py @@ -8,7 +8,10 @@ import pytest from aiida_pseudo.cli.family import cmd_family_cutoffs_set, cmd_family_show +from aiida_pseudo.data.pseudo.upf import UpfData from aiida_pseudo.groups.family import CutoffsPseudoPotentialFamily, PseudoPotentialFamily +from aiida_pseudo.groups.family.pseudo_dojo import PseudoDojoFamily +from aiida_pseudo.groups.family.sssp import SsspFamily @pytest.mark.usefixtures('clear_db') @@ -80,6 +83,30 @@ def test_family_cutoffs_set_unit(run_cli_command, get_pseudo_family, generate_cu assert family.get_cutoffs_unit() == unit +@pytest.mark.parametrize( + 'family_cls,label', [(SsspFamily, 'SSSP/1.1/PBE/efficiency'), + (PseudoDojoFamily, 'PseudoDojo/0.4/PBE/SR/standard/psp8')] +) +@pytest.mark.usefixtures('clear_db') +def test_family_cutoffs_set_established( + run_cli_command, get_pseudo_family, generate_cutoffs, tmp_path, family_cls, label +): + """Test that `aiida-pseudo family cutoffs set` is blocked for established families.""" + family = get_pseudo_family( + label=label, + cls=family_cls, + pseudo_type=UpfData, + ) + stringency = 'normal' + cutoffs = generate_cutoffs(family) + + filepath = tmp_path / 'cutoffs.json' + filepath.write_text(json.dumps(cutoffs)) + + result = run_cli_command(cmd_family_cutoffs_set, [family.label, str(filepath), '-s', stringency], raises=True) + assert f"Invalid value for 'FAMILY': The value `{family}` is not allowed for this parameter." in result.output + + def test_family_show(clear_db, run_cli_command, get_pseudo_family): """Test the `aiida-pseudo show` command.""" family = get_pseudo_family()