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

Delay or avoid keyword value conversion #55

Open
dev-zero opened this issue Jan 14, 2022 · 0 comments
Open

Delay or avoid keyword value conversion #55

dev-zero opened this issue Jan 14, 2022 · 0 comments

Comments

@dev-zero
Copy link
Contributor

Currently we're always converting keyword values to their default unit.
This is not needed for linting, only when converting to JSON or YAML and may even lead to problems when the XML schema lists internal_cp2k as the default unit, see #54.

It might be a good idea to delay the actual conversion to a time when it is actually needed and/or avoid it completely by changing the JSON/YAML format to yield a string containing the original unit.

The parser tries to normalize values here:

if current_unit != default_unit:
# interpret the given value in the specified unit, convert it and get the raw value
value = (value * current_unit).to(default_unit).magnitude
values += [value]

The reason for this is that for serialization formats like json and yaml we yield values in the CP2K default unit since we do not export any unit specification.

It's a bit unfortunate that CP2K defines the internal units as abstract internal_cp2k rather than effective unit, especially for things where there would be a physical unit behind it (other than relative tolerances where this seems to be used otherwise).
This makes external linting of input files impossible since we could only validate such values by hardcoding the effective internal unit of those parameters within cp2k-input-tools.

I guess the proper way forward is to:

  1. store an explicitly given unit for a quantity with a default unit of internal_cp2k and only validate it, do not attempt to convert the value from or to internal_cp2k.
  2. when converting to a format (json or yaml) where the unit would be lost, either yield an error if the unit was explicitly given (since we can't convert it and the recipient of the json/yaml wouldn't know which unit it is), or yield the value as a string with the unit either suffixed (1.23 angstrom) or prefixed as in the CP2K input [angstrom] 1.23.

This would also delay unit conversion to the point when it becomes necessary (for linting it is not), rather than just attempting it all the time.

The intermediate solution: if the default unit is internal_cp2k, store as is, if there is an explicit unit given, raise an error since we can't convert to internal_cp2k.

Originally posted by @dev-zero in #54 (comment)

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

No branches or pull requests

1 participant