-
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
Refactor & new analysis functions #3
Conversation
txie-93
commented
Aug 18, 2020
- Refactored the code. It now only reads the trajectory once. Significantly improved speed and memory use.
- Several new analysis functions.
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.
LGTM!
Could you add a change related to a previous PR?
I want to make a small change to the keys in REQUIRED_METADATA:
'temperature' --> 'temperature_K' (unit Kelvin)
'timestep' --> 'time_step' (follows format of other keys, i.e. individual nouns separated by _)
I believe that also requires updating all of the meta.json files in the S3 folder. Hope this is easy to do.
I like your suggestions! Yes, it is easy to do. |
Sounds good to me. I am just worried that there could be confusion on the temperature unit; unless it's understood that K is the standard unit used. |
Done! I updated both the code and the data. Merging the code now. K is quite standard in my opinion, but this is a good point. Let's keep it as it is for now, and we can change in the future if we like. |
👍 |