Move things to utils, test interchange creation #47
Conversation
|
Hello @hannahbaumann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-26 12:24:25 UTC |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| smc_components = {} | ||
| prot_settings = GromacsMDProtocol.default_settings() | ||
| # The function expects a settings dict | ||
| settings = {} |
There was a problem hiding this comment.
create_openmm_systems takes in a settings dict, which in the protocol is created using the _handle_settings function. This makes this test not so pretty, as I have to create the dictionary. Should I change the function to instead take in the actual settings objects? Or is this ok?
There was a problem hiding this comment.
Yes, I would encourage changing the method to not take in the whole dictionary unless you can avoid it. If you only rely on a couple of settings, it can make life a lot easier (if it's a whole bunch of settings then that's a different issue).
There was a problem hiding this comment.
I changed this to instead pass in the individual settings. Alternatively, I was thinking we could also have an SystemCreationSettings class that contains the settings necessary to create the OpenMM system to distinguish with the settings that are related to Gromacs. But I'm not sure if that would be confusing to the user.
hannahbaumann
changed the title
for more information, see https://pre-commit.ci
| smc_components = {} | ||
| prot_settings = GromacsMDProtocol.default_settings() | ||
| # The function expects a settings dict | ||
| settings = {} |
There was a problem hiding this comment.
Yes, I would encourage changing the method to not take in the whole dictionary unless you can avoid it. If you only rely on a couple of settings, it can make life a lot easier (if it's a whole bunch of settings then that's a different issue).
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…gromacs into move_things2utils
for more information, see https://pre-commit.ci
…gromacs into move_things2utils
for more information, see https://pre-commit.ci
…gromacs into move_things2utils
Description
This PR seeks to address the following issues:
create_interchangeand_assign_partial_chargesto their own utilities #30mdout.mdpis being written outside ofsharedandscratchdirectories. #53