Store unit system in H5MD file #3751
Conversation
It cannot be disabled with `# noqa: E701` and can cause formatted code to throw a SyntaxError.
Codecov Report
@@ Coverage Diff @@
## python #3751 +/- ##
======================================
- Coverage 89% 89% -1%
======================================
Files 552 552
Lines 24532 24560 +28
======================================
+ Hits 21861 21880 +19
- Misses 2671 2680 +9
Continue to review full report at Codecov.
|
|
Several CI images are missing the |
|
@KaiSzuttor would you like to sign off on these changes? This PR makes use of If we don't want to add this |
There was a problem hiding this comment.
In general I think we should not write our own unit system code. So what may make sense in the future is to use Pint and derive the strings that need to be written to the file automatically.
Looks good to me for now. Thanks for the contribution!
| @@ -18,7 +18,7 @@ repos: | |||
| always_run: false | |||
| files: '.*\.(py|pyx|pxd)' | |||
| exclude: '\.pylintrc|.*.\.py\.in' | |||
| args: ["--ignore=E266,W291,W293", "--in-place", "--aggressive"] | |||
| args: ["--ignore=E266,E701,W291,W293", "--in-place", "--aggressive"] | |||
There was a problem hiding this comment.
E701 causes dataclasses members to be formatted on a new line:
- force: str = dataclasses.field(
- init=False, default='')
+ force:
+ str = dataclasses.field(
+ init=False, default='')This change causes a syntax error:
1559 File "/builds/espressomd/espresso/build/src/python/espressomd/io/writer/h5md.py", line 36
1560 force:
1561 ^
1562 SyntaxError: invalid syntax
E701 cannot be disabled on a per-line basis. I tried using line continuation symbols \ to work around it, without success. I think we should permanently disable it, even if we remove dataclasses, because it might cause us trouble again in the future, and troubleshooting this error took me a while.
| id (implies serial write). | ||
| id (implies serial write) | ||
|
|
||
| - ``unit_system``: optionally, physical units for time, mass, length and |
There was a problem hiding this comment.
what about other SI base units that are defined in the H5MD documentation?
There was a problem hiding this comment.
The other SI units (current, temperature, moles, candela) do not seem to apply for espresso particles. The per-particle temperature is more of a trick for tailored Langevin dynamics. Same thing for derived units. Torque could have been relevant, but isn't defined.
|
Concerning the dataclasses, I think it's not worth the dependency overall because the respective constructor would not be a lot of additional code and the other provided functions like |
This commit can be reverted once ESPResSo requires python 3.7 as the minimal version. In 3.7, dataclasses is part of the Python Standard Library.
|
|
||
| def __post_init__(self): | ||
|
|
||
| def __init__(self, **kwargs): |
There was a problem hiding this comment.
you could still use type annotations if you wanted to...
There was a problem hiding this comment.
there's no need to re-implement dataclasses :) providing a different type already throws a meaningful error message: RuntimeError: Provided argument of type int is not convertible to std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >
Add option to define and store the unit system used in the simulation according to the H5MD module units defined here: https://nongnu.org/h5md/modules/units.html#unit-string
Description of changes:
h5md.H5md:unit_systemunitsif the unit system is set