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

Velocity and geometry units added to output files. #200

Merged
merged 4 commits into from
Jan 30, 2025
Merged

Conversation

JanosJiri
Copy link
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
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.

src/analysis.F90 Outdated
@@ -178,7 +180,7 @@ subroutine velout(vx, vy, vz, time_step)
integer :: iat, iw

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
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
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
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!

Copy link

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
Contributor Author

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

Copy link
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
19 checks passed
@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