-
Notifications
You must be signed in to change notification settings - Fork 3
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
Document charge assignment hierarchy #68
Comments
Working on this has surfaced #69 |
I agree with everything @mattwthompson said originally and would approve a PR to clarify this in the spec early in the |
Fully agree with this as well, @mattwthompson . Nice catch, agree it needs to be much clearer. Same perspective as Jeff. |
Definitely agree as well -- this would be a huge improvement to the standard! Would it be helpful to document the isomorphism issue here too? |
I took some bits of my write-up and turned them into Interchange docs which I think covers in information content what we seem to agree is missing here. I probably won't have time to do this for a few weeks, but this could be used as a starting point for what we want added into the spec here. |
The OpenFF Toolkit/Interchange has/had long-standing implementations of hierarchical charge assignment, in particular that charges are assigned from methods in the following order
charge_from_molecules
kwargThis is in the spec ... but also not presented in a helpful way. I found (detailed below the fold) hints at this hierarchy in each section, but it'd be really convenient to users to have this hierarchy spelled out. I happen to know this behavior because of my familiarity with the implementation, but I'd be really confused if I was a new user trying to figure out which of the many possible charge assignment methods was actually happening. (Or see another failure case below.)
I think, maybe under this section header, a short mention of the above list would be helpful to users. Most of this information is buried in the other sections but not presented in a way that makes it explicit.
As a somewhat problematic example, imagine a user has a polymer of some sort in which AM1 is not practical, there aren't pre-specific charges for
charge_from_molecules
, there also isn't a charge increment model, and they made a typo in a large SMIRKS pattern describing the library charges of a residue. A scientist is probably only expecting pre-specific or library charges to be used on macromolecules, so our software hanging (the result of firing up AM1) is certainly useless and arguably surprising. This could be worked around, and the toolkit tries (see below) to warn users if AM1-BCC is called on large molecules, but the result in this scenario is confusion as to why AM1-BCC is even in the picture.Library charge section
Note that this isn't true, since
charge_from_molecules
takes precedence over library charges.Charge increments section
AM1-BCC section
“Prespecified charges” section doesn’t even communicate “if you set charges this way, all other methods are skipped” and I really think it should
The text was updated successfully, but these errors were encountered: