marrink-lab / polyply_1.0 Goto Github PK

Currently the linear string sequence only takes integer monomer numbers per block. But there should be a way to generate a random sequence from fractions. Question is: Should we make a separate grapher tool or still implement this as interpretation of the string. I feel the separate tool would be the clearer option. Should be perhaps offload all the graphing work into a sperate tool and only read json files?

Not sure if it's really an issue but I wondered this in other occasions myself and I figured I would bring it up here so we can discuss it and who knows can clear this out. It was my understanding that function type 1 for the periodic function for proper dihedrals in Gromacs allows only one multiplicity term. So that, if one wants to use a proper dihedral function composed by several multiplicity terms, one should use the function type 9 (see also the Table from the Gromacs manual with all the bonded terms). Now, is it so? If it is, the PEO model would need to use function type 9 for the dihedral (instead of 1 as shown below), wouldn't it?

[ dihedrals ]
 1  2  3  4 1 180.00 1.96 1
 1  2  3  4 1 0 0.18 2
 1  2  3  4 1 0 0.33 3
 1  2  3  4 1 0 0.12 4

but I am not sure I am understanding correctly the Gromacs manual, which talks about [ dihedraltypes ] too... (see first link).

I'm pretty sure you could dump this in a [molecules] (or [system], whatever it's called. Can't remember) section parser method, but meh.

Originally posted by @pckroon in https://github.com/_render_node/MDE3OlB1bGxSZXF1ZXN0UmV2aWV3NDAxNzcwNTc4/pull_request_reviews/more_threads

All source files and License file

Currently all molmeta is ignored except for "explicit". But we need some form of molmeta integration to deal with ScFix of M3 proteins etc.

Clean up the way edges are generated in the different processors

Currently chirality for CHARMM and alike force-fields are not supported. The same is true for general structure generation. When the CHARMM molecule contains many competing dihedral terms minimization frequently fails, leading to poor structures being generated.

this line fails if vermouth detects more than one molecule in that case we get a list not a single molecule

also reading in complete molecules fails, because of the last_atom definition

also somehow assignment of coordinates to the meta-molecule fail for molecules probably with same resid?! This function needs a lot more testing.

Though pragmas are parsed supposedly in correct fashion they don't show up when writing the itp file.

Special bug: If a link for a bond is defined as shown below it will be applied only up to the nth-1 residue. That means molecules with only two residues will not have a link applied at all.

[link]
[bonds]
atom1  -atom2

The reason lies in this line of code:

Here we tether the generation of the residues that correspond to the link to the 0th node key of an edge. Overall this means links need to be written in a 'forward' thinking manner, which is the case for most molecules. Either we fix this or this needs to be documented but I'll need to investigate a bit more how much this problem arises for angles and dihedrals.

I don't really like passing the args object. I'd prefer splitting it up into the relevant pieces.

Originally posted by @pckroon in #29

The try is too broad for this except. I suggest:

try:
    parser = getattr(MetaMolecule, 'from_{}'.format(extension))
except AttributeError as error:
    raise IOError(...) from error
else:
    meta_molecule = parser(...)

Also, the parser kwargs should probably not be named 'json_file'

Originally posted by @pckroon in #19

https://github.com/fgrunewald/polyply_1.0/blob/e3d0148de2ea4ce4bdd92404937a6eba5197212d/polyply/src/apply_links.py#L232-L233

The above code assumes that a term with length n can be at most n nodes away in the residue graph. This is true but can lead to confusion when the term contains order parameters of length smaller than n.

Different constructors set different attributes depending on the use case. They should be unified and always create the same attributes.

With respect to #4 it a sensible header should be written to the itp files

Currently GenSeq command line parsing of arguments fails

I encounter the same issue as here (jbarnoud/cgbuilder#2) with the gro file generated by running a command like:

polyply gen_coords -p system.top -o P3HT_test.gro -name P3HT

So, as Jonathan did there, I think adding a "new line" command after the box dimensions on the last line of the gro file should fix the issue.

https://github.com/fgrunewald/polyply_1.0/blob/e3d0148de2ea4ce4bdd92404937a6eba5197212d/polyply/src/apply_links.py#L304-L318

This code actually (almost) always evaluates to true, because we already checked that all resids are part of the residue graph and a path is at most of length 4, we almost always find a fitting subgraph. Instead we should check that all nodes and edges are part of the meta-molecule graph or that the path is part of the graph.

@pckroon Do you know a nx function that would do this.

In a nutshell it should do:

for node in graph2.nodes:
        if not node in graph1.nodes:
           return False
for edge in graph2.edges:
        if not edge in graph1.edges:
           return False
return True

Currently has wrong executable

Currently we cannot do modifications. This also depends on marrink-lab/vermouth-martinize#229 to be merged into master.

After creating a virtual environment I was able to download all of the requisite source code and install vermouth and tqdm. However, when trying to install polyply using setup.py I received this error message:

"ERROR: ./setup.py is not a valid editable requirement. It should either be a path to a local project or a VCS URL (beginning with svn+, git+, hg+, or bzr+)."

Come up with some organization of the library to merge vermouth force-field with polyply input in a sensible fashion. Things to think about:

• import rtps from gromacs libraries if GMXRC is sourced
• organize by AA, CG or rather ff-files polyply etc.
• add some form of licenses and citations to these files

It should be possible to construct a MetaMolecule from anything that is in nx.readwrite and can set node attributes. We could make a meta-calls that basically collects the constructor classes. But this is not high priority at the moment.

Remove all things like pychache and files that are already in the .gitignore but still got into the repro.

When using library files it might be users try to combine two elements for which there is no link. We should print a warning telling them that this is dangerous.

If you want to typehint this completely (optional), you can do something like templates: dict[str, vermouth.molecule.Block], indicating its keys are strings and values are Blocks. You'll still need to tell the str is a resname.
And I'm not sure how much sphinx/autodoc/apidoc likes the not-4-spaces indent. But that we can fix later

Originally posted by @pckroon in #29

Check that all stuff for M3 proteins is handled accurately.

Implement checking of residue isomorphism before generating the templates. All residues need to have a unique name and users u sally don't pay attention to this.

I suggest passing the file handle as argument, rahter than the filepath. That way reading e.g. compressed files or network streams becomes possible rather than painful

Originally posted by @pckroon in #29

Docstrings need to either be added or put into proper format

There are quite a few test_molecules and test_systems. Some of those can probably be imported form a single helper file and be the same system. This is some maintenance that should be done.

Don't include file if in ifdef statement and def not set

Organize the multiple executables in a intuitive manner like gmx tools.

Martini topology files currently are not parsed correctly, because it now requires all parameters to be there.

Apply Links needs more testing, especially more unit tests

Unless the bonds are ordered in the itp file our intial structure generation fails. This needs to be improved by using the graph properties and constructing by following the edges.

The following line adds resids to nodes but depends on the ordering of the dict.

https://github.com/fgrunewald/polyply_1.0/blob/7184dba945d98f7f2dbd9493b147edc099838f22/polyply/src/apply_links.py#L84

It should be replaced by some more robust way to parse this information.

Create a logger, and actually log a warning. That gives you (and the end user) much more control over which messages to show, and two maybe write them to a file as well

Originally posted by @pckroon in https://github.com/_render_node/MDE3OlB1bGxSZXF1ZXN0UmV2aWV3NDIyNTM4MTgx/pull_request_reviews/more_threads

This is very expensive. How about making vector_bundle a set (or dict), and popping random items from it?

Originally posted by @pckroon in https://github.com/_render_node/MDE3OlB1bGxSZXF1ZXN0UmV2aWV3NDE2ODY3OTAx/pull_request_reviews/more_threads