RecommendedCutoffMixin: store the unit of cutoffs in extras
#57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mbercx I served you a cold one! 🍺 |
mbercx
requested changes
Apr 8, 2021
| @@ -107,16 +128,19 @@ def set_cutoffs(self, cutoffs: dict, default_stringency: str = None) -> None: | |||
| :param default_stringency: the default stringency to be used when ``get_recommended_cutoffs`` is called. If is | |||
| possible to not specify this if and only if the cutoffs only contain a single stringency set. That one will | |||
| then automatically be set as default. | |||
| :param unit: TODO | |||
By storing the unit of the cutoffs values explicitly in the extras, it not only will give more certainty to the user of what the cutoffs mean because they can inspect the unit, but the `get_recommended_cutoffs` method can also provide the `unit` argument to automatically convert it to another unit of energy if so desired. The conversion between units is provided by the `pint` library, which is added as a dependency. Users can specify units in the API through strings using the names that are recognized by `pint`. Both the shorthand, e.g. `eV`, as well as the long version `electron_volt` can be used. Other common energy units are `Ry` and `Eh`, for Rydberg and Hartree atomic units respectively. The default unit that is set when defining recommended cutoffs is still set to be electron volt, which was the implicitly assumed unit uptil now. Families that have been created with versions preceding this commit, will not actually have the unit defined in the unit, but `get_cutoffs_unit` will return the default electron volt if it is missing. This does mean that the `RecommendedCutoffMixin`DEFAULT_UNIT` cannot be changed or this would lead to old families to be interpreted to have the incorrect unit.
sphuber
force-pushed
the
fix/047/add-recommended-cutoff-units
branch
from
3686caa to
24eb652
Compare
mbercx
previously approved these changes
Apr 9, 2021
| @@ -143,13 +169,17 @@ def get_recommended_cutoffs(self, *, elements=None, structure=None, stringency=N | |||
| :param stringency: optional stringency if different from the default. | |||
| :return: tuple of recommended wavefunction and density cutoff. | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #47
By storing the unit of the cutoffs values explicitly in the extras, it
not only will give more certainty to the user of what the cutoffs mean
because they can inspect the unit, but the
get_recommended_cutoffsmethod can also provide the
unitargument to automatically convert itto another unit of energy if so desired.
The conversion between units is provided by the
pintlibrary, which isadded as a dependency. Users can specify units in the API through
strings using the names that are recognized by
pint. Both theshorthand, e.g.
eV, as well as the long versionelectron_voltcan beused. Other common energy units are
RyandEh, for Rydberg andHartree atomic units respectively.
The default unit that is set when defining recommended cutoffs is still
set to be electron volt, which was the implicitly assumed unit uptil now.
Families that have been created with versions preceding this commit,
will not actually have the unit defined in the unit, but
get_cutoffs_unitwill return the default electron volt if it ismissing. This does mean that the
RecommendedCutoffMixin.DEFAULT_UNITcannot be changed or this would lead to old families to be interpreted
to have the incorrect unit.