Non-deterministic charge validation - #2132
Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2132 +/- ##
==========================================
- Coverage 95.13% 90.48% -4.65%
==========================================
Files 213 213
Lines 20878 20930 +52
==========================================
- Hits 19862 18939 -923
- Misses 1016 1991 +975
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No API break detected ✅ Griffe output |
| def benzene_charged_system(self, benzene_modifications): | ||
| return ChemicalSystem({"ligand": benzene_modifications["benzene"]}, name="charged") | ||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
Would it make sense to also add a test where the first smc is charged and the second does not have charges? How would we want to handle such a case?
|
|
||
| for smc in smcs: | ||
| offmol = smc.to_openff() | ||
| if offmol.partial_charges is not None and np.any(offmol.partial_charges): |
There was a problem hiding this comment.
Would we want to support a case where the user supplies partial charges will all zeros?
There was a problem hiding this comment.
+1 - given we have our own benchmark case where we want to do this, we probably should support it
There was a problem hiding this comment.
Yes good idea. Currently, this only works as a Library charge would we want to keep that as a source of protection to make sure users know what they are doing or allow it as charges on the molecule as well?
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
I think this matches what we discussed yesterday - as you mentioned, we should be able to drop the partial charge assignment calls in Protocols with this.
|
|
||
| for smc in smcs: | ||
| offmol = smc.to_openff() | ||
| if offmol.partial_charges is not None and np.any(offmol.partial_charges): |
There was a problem hiding this comment.
+1 - given we have our own benchmark case where we want to do this, we probably should support it
3 participants
Fixes #2115 by adding a validation method that attempts to catch partial charge generation methods which are used at runtime and are nondeterministic.
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
If yes, please provide details here: No
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin