-
Notifications
You must be signed in to change notification settings - Fork 32
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 GRO writer #48
Add GRO writer #48
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.
Small changes and comments
b749b57
to
1810394
Compare
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.
Given that the gro writer will be an ongoing work in progress for a long time, do we want to throw warnings about the limited res_id, res_name, box functionality?
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.
Looks like a good milestone for a gro writer
@justinGilmer just want to you look over how I'm handling the vectors, with the understanding it will be properly fixed later. Feel free to merge if it looks good to you. |
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.
Looking great! Just a few nits and questions.
added unit vector and scaled vector support with better names under #58. |
All tests passing for me, and the result is the same as above. Can you pull this locally, run tests, and if everything's good, merge? @justinGilmer |
will do! |
Will require at least #40 and #49