Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Charmm parameters writer #848
base: main
Are you sure you want to change the base?
Charmm parameters writer #848
Changes from 6 commits
3327de2
25f893f
13e0d6e
2c7df9c
7bfbb3f
ea5ec28
2f10015
362f747
86b6d06
2f7657e
9b4c3e7
d107c21
d150a31
2fb6ffe
2fab3c0
c325111
a3c0d42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we need to make sure we're using this everywhere we write out parameters? I see we use it for the atom mass, but not anywhere else.
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, for in order to handle any input unyts this will do the filtering. I was just too lazy to do it every time at first. Was wondering if you think we should make a system that isn't specific to LAMMPS for this one. It is the exact same as the real units, but maybe a touch confusing to copy them across to other engines. What do you think?
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 wonder if
unit_system
should be a parameter ofwrite_prm
that defaults toNone
. If nothing is passed, then all units are used as-is from the toplogy? If we setunit_system = topology.unit_system
as default behavior, would that work as well?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.
The PRM file looks to me like it has default assumed units of kCal/mol, angstrom, amu, radians, etc for it's units. So it can only convert to one defined system.
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.
This doesn't need to be fixed here, but I feel like this attribute should be more readable rather than just knowing that 0th index is LJ and 1st index is electrostatics. Maybe a dict? I can open a separate issue.
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.
Was thinking the same thing myself. I think it should also be {"electrostatics":{"1-4":0.5}}, instead of a fixed list. Could potentially be some odd 1-5 interactions at some point.
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.
Yeah, that's sort of what I was thinking as well.