Skip to content

Velocity and geometry units added to output files.#200

Merged
JanosJiri merged 4 commits intomasterfrom
units_in_output
Jan 30, 2025
Merged

Velocity and geometry units added to output files.#200
JanosJiri merged 4 commits intomasterfrom
units_in_output

Conversation

@JanosJiri
Copy link
Copy Markdown
Contributor

@JanosJiri JanosJiri commented Jan 23, 2025

The units of geometries and velocities are lacking in their respective output files. I've spent time looking for the right units of velocities in ABIN several times (because I always forget) and others in Prague have the same issue. I suggest to add the units of geometries and especially velocities to their respective output files.

The proposed way adds the units to every single geometry/velocity such that if someone greps specific geometries/velocities, the information about the unit is kept. Alternatively, the unit could be added to a header, but this would add one extra line and could break loads of existing scripts grepping from movie.xyz or velocities.xyz. What I propose is left for discussion @danielhollas , but adding this would save a lot of time for many people.

FYI, all the tests are failing because of the added units to the files but I did not mend the tests before (or if) we agree on the precise way of adding them.

Copy link
Copy Markdown
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I basically agree with all you wrote, and the approach looks reasonable. I'll think a bit more about the wording/ formatting later today.

Comment thread src/analysis.F90 Outdated

write (UVELOC, '(I0)') natom
write (UVELOC, '(A,I0)') 'Time step: ', time_step
write (UVELOC, '(A,I12, A)') 'Time step: ', time_step, ' Vel. units: a.u.'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would ideally want to avoid using the fixed-width for the integer formatting. I am assuming the compiler complained once you added an extra string after it? Should we move the units at the beginning of the line? Another question, perhaps just units: a.u. would be sufficient? I guess it should be obvious from the context that these units are for velocities?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment, would you agree on write (UVELOC, '(A,I0)') 'Units: a.u.; Time step: ', time_step? Same for movie.xyz of course.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, let's go with that, thanks!

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.84%. Comparing base (eef82e1) to head (d4a87f6).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #200   +/-   ##
=======================================
  Coverage   92.84%   92.84%           
=======================================
  Files          47       47           
  Lines        6761     6761           
  Branches      764      764           
=======================================
  Hits         6277     6277           
  Misses        472      472           
  Partials       12       12           
Flag Coverage Δ
unittests 20.37% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/analysis.F90 98.85% <100.00%> (ø)

@JanosJiri JanosJiri marked this pull request as ready for review January 29, 2025 13:38
@JanosJiri
Copy link
Copy Markdown
Contributor Author

This PR (number 200!) is ready for review @danielhollas.

Copy link
Copy Markdown
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, thanks!

Ideally, we'd have consistent headers and include the simulation time in velocities as well, and similarly for forces. But let's leave this for future, I know it was probably very annoying to fixup all the reference files. 😅

@JanosJiri JanosJiri merged commit c9eed65 into master Jan 30, 2025
@JanosJiri JanosJiri deleted the units_in_output branch January 30, 2025 12:10
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

Successfully merging this pull request may close these issues.

2 participants