-
Notifications
You must be signed in to change notification settings - Fork 5
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
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.
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.' |
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.
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?
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.
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.
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.
Yep, let's go with that, thanks!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR (number 200!) is ready for review @danielhollas. |
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.
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. 😅
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.