Skip to content
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

Generalize ff interaction #360

Merged
merged 24 commits into from
Feb 15, 2024
Merged

Generalize ff interaction #360

merged 24 commits into from
Feb 15, 2024

Conversation

ehhartmann
Copy link
Collaborator

@ehhartmann ehhartmann commented Jan 31, 2024

Deal with problems discussed in #359 and facilitate future addition of non-amber force-fields to kimmdy.

  • expose reference to residuetypes file in config
  • add optional radicals section to config
  • add tests

@ehhartmann
Copy link
Collaborator Author

2 failed, 94 passed, ready for review

@jmbuhr
Copy link
Collaborator

jmbuhr commented Feb 6, 2024

Which are the failing tests? tox looks fine. I'd advise to run black before the review so that we have common formatting, otherwise we might get confused with line numbers during it.

@ehhartmann
Copy link
Collaborator Author

Integration as always. It's formatted with the latest black version now. To me the docs action failure looks like some parts of the action are outdated.

@ehhartmann
Copy link
Collaborator Author

To use the new features of this PR, the plugins (at least both HAT ones) would have to be updated as well

@jmbuhr
Copy link
Collaborator

jmbuhr commented Feb 6, 2024

ah, looks like the doc parser is now attempting to parse every heading in the docstring as a proper semantic section, but https://github.com/hits-mbm-dev/kimmdy/blob/ce1daf0e68627c04e16c878e0d4d8f897ad5c833/src/kimmdy/parsing.py#L108-L109 is just a random heading I threw in, so this just has to not be a section by removing the ---.

@jmbuhr
Copy link
Collaborator

jmbuhr commented Feb 6, 2024

would you like me to do small fixes directly in the PR when I review or just comment instead?

@ehhartmann
Copy link
Collaborator Author

Just change it

@ehhartmann ehhartmann requested review from jmbuhr and removed request for jmbuhr February 7, 2024 12:38
Copy link
Collaborator

@jmbuhr jmbuhr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good, just some minor questions I can then also fix myself if you approve.

src/kimmdy/coordinates.py Show resolved Hide resolved
src/kimmdy/kimmdy-yaml-schema.json Outdated Show resolved Hide resolved
src/kimmdy/kimmdy-yaml-schema.json Show resolved Hide resolved
src/kimmdy/runmanager.py Show resolved Hide resolved
src/kimmdy/tools.py Show resolved Hide resolved
src/kimmdy/topology/ff.py Show resolved Hide resolved
src/kimmdy/topology/topology.py Outdated Show resolved Hide resolved
src/kimmdy/topology/topology.py Show resolved Hide resolved
@ehhartmann
Copy link
Collaborator Author

For radicals, the workflow is different between writing radicals: '' and not having a radicals section at all (#59) because in the missing section case, we use the bondorder to guess radicals. For the modify-top tool I thought it was a reasonable default to say there are no radicals but I wouldn't do that for a kimmdy run. If you want to have it consistent, I would have no default in both.

Regarding residuetypes, it is similar. No section means we want to use the one in the force field. Having aminoacids.rtp as default would mean we need it in the rundir.

Feel free to rename the functions for finding and setting the atoms attribute. If you find ways to make the same logic more elegant I would be happy. It felt quite awkward to do it like this.

@jmbuhr jmbuhr added testthis and removed testthis labels Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants