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

add warning when mod not found #556

Merged
merged 4 commits into from
Oct 23, 2023
Merged

Conversation

csbrasnett
Copy link
Collaborator

Addresses #553

@pckroon
Copy link
Member

pckroon commented Oct 20, 2023

Thanks! I appreciate how subtle and small your solution is :)
It does cause some test failures though, could you take a look at those? And could you add a test that the warning gets issued?

@csbrasnett
Copy link
Collaborator Author

Thank you Peter! And thanks to @jan-stevens for help with fixing the tests :)

Comment on lines 228 to 231
if mod_found == False:
LOGGER.warning('{} with resid {} not found. '
'No modification made.'
''.format(resspec['resname'], resspec['resid']))
Copy link
Member

Choose a reason for hiding this comment

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

Small problem actually. Not all resspecs need to have a resname or resid attribute. Might be better to format the respec like the CLI accepts it. You should be able to use _format_resname for that. Also that function has a bad bad name. Pretty sure that's my fault though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the heads up on _format_resname, will update using that.

vermouth/tests/test_annotate_mut_mod.py Outdated Show resolved Hide resolved
Comment on lines 294 to 295
modification = [({'resname': 'cter', 'resid': 3}, 'C-ter'),
({'resname': 'nter', 'resid': 1}, 'N-ter')]
Copy link
Member

Choose a reason for hiding this comment

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

Right, as in the comment before, not all resspecs need a resid or resname. These for example don't need a resid; the 'nter' and 'cter' already have enough information to not need the resid or sane resname.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

weirdly, adding those resids seemed to be what fixed the tests before, but I just tried again with them removed and it looks like it works without? I think we thought it was overwriting the whole dictionary now, and that's why they were needed. Not sure what's happened there.

@pckroon
Copy link
Member

pckroon commented Oct 23, 2023

This looks good to me!
The tests failed since your first warning assumed a resid (and resname) would always be available in a resspec. Since the _format_resname doesn't make that assumption, no more test failure.

@pckroon pckroon merged commit 1593bf1 into marrink-lab:master Oct 23, 2023
8 checks passed
@csbrasnett csbrasnett deleted the mutate-fix branch October 23, 2023 12:09
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.

2 participants