marrink-lab / vermouth-martinize

Describe and apply transformation on molecular structures and topologies
Apache License 2.0
84 stars 37 forks source link

Multiple mutate fixes #565

Closed csbrasnett closed 2 months ago

csbrasnett commented 6 months ago

re-addresses #553 in the case that mutations are given across multiple chains. This PR makes sure that in the case that multiple chains are targeted, the check for the target residue existing is not present across all chains. So warnings aren't raised when the 'missing' residue is encountered.

pckroon commented 2 months ago

Updating the codecov action seems to have fixed the failing test thingie

csbrasnett commented 2 months ago

just to check, are you expecting any more changes from me?

csbrasnett commented 2 months ago

I think this still gives erroneous warnings for chemically disconnected things. For example, protein with 2 chains (A and B) which are not connected by a chemical bond (and are thus separate Molecules), combined with a valid resspec that defines a chain.

If I've understood your concerns correctly, hopefully the two new tests satisfy them?

csbrasnett commented 2 months ago

So, I think those tests do what you expect them to, but I'm not sure they should work, as they don't pick up on the error identification correctly? Eg, GLU4:GLY should mutate any GLU at resid 4 on every chain, but if resid 4 is not GLU then there should be a warning, and there currently isn't. I'll have a think about how to address this.

I'll also update the martinize2 help to add some more detail about what's possible with the mutate/modify flags?

pckroon commented 2 months ago

Eg, GLU4:GLY should mutate any GLU at resid 4 on every chain, but if resid 4 is not GLU then there should be a warning, and there currently isn't.

Exactly :) I think you'll need to adapt the run_system method to track whether at the end there are any resspecs that didn't match anything

I'll also update the martinize2 help to add some more detail about what's possible with the mutate/modify flags?

This is always welcome. Documentation is not my strong suite

csbrasnett commented 2 months ago

Eg, GLU4:GLY should mutate any GLU at resid 4 on every chain, but if resid 4 is not GLU then there should be a warning, and there currently isn't.

Exactly :) I think you'll need to adapt the run_system method to track whether at the end there are any resspecs that didn't match anything

Isn't this already captured by the warning raised?

pckroon commented 2 months ago

That warning works for single molecules, not the entire system, which is what's going wrong if you have a resspec that does not mention any chains but does not match anywhere

csbrasnett commented 2 months ago

That warning works for single molecules, not the entire system, which is what's going wrong if you have a resspec that does not mention any chains but does not match anywhere

I'm pretty sure that possibility's covered by this test though:

[
    {'chain': 'A', 'resname': 'ALA', 'resid': 1},
    {'chain': 'A', 'resname': 'ALA', 'resid': 2},
    {'chain': 'A', 'resname': 'ALA', 'resid': 3},
    {'chain': 'B', 'resname': 'ALA', 'resid': 1},
    {'chain': 'B', 'resname': 'ALA', 'resid': 2},
    {'chain': 'B', 'resname': 'ALA', 'resid': 3}
],
[(0, 1), (1, 2), (3, 4), (4, 5)],
[({'resname': 'GLY', 'resid': 1}, 'ALA')],
True

ie. two molecules in the system, target any GLY with resid 1 (of which there are none), and expect a warning to be raised

pckroon commented 2 months ago

Since the test was always running on a single Molecule (rather than a System of multiple molecules) it was hiding the issue I was trying to dig up. The problem is that if a resspec does not get applied to /all/ molecules, a warning gets issued, even if it applies to at least 1.

csbrasnett commented 2 months ago

thanks for that additional test, I see what you mean now about what wasn't captured. Will fix in due course!

csbrasnett commented 2 months ago

you're right, I think I had confused something between that and test 8 sorry. I think now this should address the different possible cases, it was a bit of a headache getting something that satisfied both test 7 and test 8 together.

pckroon commented 2 months ago

No worries. I know how these things can happen. My suggestion is to simplify the annotate_modifications function again, to simply apply yes or no depending on whether the resspec was matched anywhere. Being able to warn whether a modification applied to just one or all chains is nice to have, but not necessary. The most relevant message is when a modification doesn't apply anywhere. I'll leave it up to you whether the more detailed message is worth the added complexity.