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

Add units #378

Merged
merged 11 commits into from
Jan 14, 2025
Merged

Add units #378

merged 11 commits into from
Jan 14, 2025

Conversation

ElliottKasoar
Copy link
Member

@ElliottKasoar ElliottKasoar commented Jan 7, 2025

Resolves #131 and #277

  • Creates dictionary of units shared by all calculations, to save relevant items when writing out files
  • Renames time_fs to time, as this is labelled now by the units.
  • Pins default ASE units to 2014, matching current default

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

tests/test_md_cli.py Outdated Show resolved Hide resolved
janus_core/calculations/base.py Outdated Show resolved Hide resolved
@ElliottKasoar ElliottKasoar linked an issue Jan 8, 2025 that may be closed by this pull request
@ElliottKasoar ElliottKasoar added the enhancement New/improved feature or request label Jan 8, 2025
oerc0122
oerc0122 previously approved these changes Jan 10, 2025
Copy link
Collaborator

@oerc0122 oerc0122 left a 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!

@ElliottKasoar
Copy link
Member Author

One slightly messy aspect is this effectively adds a second, slightly different version of the MD units dictionary. These should probably be combined.

The stats units now use the units dictionary in Base, which means they should all be consistent now (e.g. Angstrom was referred to as A in stats, but Ang otherwise).

@ElliottKasoar ElliottKasoar marked this pull request as ready for review January 14, 2025 14:13
oerc0122
oerc0122 previously approved these changes Jan 14, 2025
Copy link
Collaborator

@oerc0122 oerc0122 left a 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?

janus_core/calculations/md.py Show resolved Hide resolved
@ElliottKasoar
Copy link
Member Author

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.

@oerc0122
Copy link
Collaborator

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.

@ElliottKasoar ElliottKasoar merged commit 43c8632 into stfc:main Jan 14, 2025
13 checks passed
@ElliottKasoar ElliottKasoar deleted the add-units branch January 14, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New/improved feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add units to info units
2 participants