Lazy merge #589
Lazy merge #589
Conversation
|
If/once you've merged the codecov update in #565 I'll merge main into here and I guess that'll fix the tests |
bin/martinize2
Outdated
| elif "all" in args.merge_chains and len(args.merge_chains) == 1: | ||
| vermouth.MergeChains(chains=None, all_chains=True).run_system(system) |
There was a problem hiding this comment.
args.merge_chains cannot be 'all' and have a length of 1 right? Since the split only happens in the if
There was a problem hiding this comment.
this is defending against multiple arguments though, eg. if you give both -merge all -merge A,B then args.merge_chains = ['all, 'A,B'], so really you need to make sure there really is only one argument
There was a problem hiding this comment.
Good catch! Needs a comment though.
vermouth/processors/merge_chains.py
Outdated
| def __init__(self, chains=None, all_chains=False): | ||
| self.chains = chains | ||
| self.all_chains = all_chains |
There was a problem hiding this comment.
| def __init__(self, chains=None, all_chains=False): | |
| self.chains = chains | |
| self.all_chains = all_chains | |
| def __init__(self, chains=None, all_chains=False): | |
| self.chains = chains or [] | |
| self.all_chains = all_chains |
- made stronger warnings in martinize2 - clarified conditions for merging in merge_chains - improved warnings and exceptions in merge_chains - changed tests to reflect abovex
vermouth/processors/merge_chains.py
Outdated
| def __init__(self, chains=[], all_chains=False): | ||
| self.chains = chains | ||
| self.all_chains = all_chains |
There was a problem hiding this comment.
It's generally a bad idea to give a mutable type as default argument, since it will be created only once and shared among all calls/instances. Here it's probably fine since chains is not modified anywhere, but still, something like this is preferred:
def __init__(self, chains=None, all_chains=False):
self.chains = chains or []
self.all_chains = all_chains.As an example of what can go wrong:
def a(x=[]):
x.append(1)
print(x)
a()
a()
vermouth/processors/merge_chains.py
Outdated
| _chains = set() | ||
| for molecule in system.molecules: | ||
| # Molecules can contain multiple chains | ||
| _chains.update(node.get('chain') for node in molecule.nodes.values()) | ||
| else: | ||
| LOGGER.warning('Conflicting merging arguments have been given. Will abort merging.') | ||
| return | ||
| raise ValueError |
There was a problem hiding this comment.
Do we want an informative message here?
There was a problem hiding this comment.
Have put one, but probably not necessary. Martinize2 will raise its ArgumentError before it gets here if both have been given, so this would have to be someone using the processor on its own for some reason!
There was a problem hiding this comment.
Exactly that, but that is a possibility we want to enable/encourage/support
|
Excellent work, thanks! |
Lazy merging of chains as per #587. Instead of specifying groups of chains to merge, users can just give
-merge alland all chains get merged.