-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add units #378
Add units #378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, I approve of adding proper units. One consideration is whether we want to make an issue and add support for pint
to handle the units for us and allow users to dump them however they want.
This might be a major piece of work though and again quite pervasive throughout the code. Definitely another PR, but this is a positive change!
Co-authored-by: Jacob Wilkins <[email protected]>
The stats units now use the units dictionary in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Only other consideration I can think of (having written a unit-handler again recently), do you think Janus will ever need to offer alternative unit schemes or is that someone else's problem?
I can imagine it as an eventuality, but I don't see it happening particularly soon. Perhaps some sort of conversion tool for input/output files would be a first step anyway. |
That boils down to "someone else's problem" for now. Happy with that. |
Resolves #131 and #277
time_fs
totime
, as this is labelled now by the units.One slightly messy aspect is this effectively adds a second, slightly different version of the MD units dictionary. These should probably be combined.
As with all current PRs, this depends on #376 or #377 for tests to pass