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

Initial PolymerProposalEngine #64

Merged
merged 24 commits into from
Jan 22, 2016
Merged

Initial PolymerProposalEngine #64

merged 24 commits into from
Jan 22, 2016

Conversation

juliebehr
Copy link

Does not select mutants; just constructs new topologies assuming mutations specified in metadata are exactly what need to be made

@jchodera
Copy link
Member

Yay!

@jchodera
Copy link
Member

Where did the PDB templates come from? Do we need those for the atom names/order, or the geometry?


import simtk.openmm.app as app
from collections import namedtuple
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

I think this is causing the build failure on travis

Copy link
Member

Choose a reason for hiding this comment

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

Oh, there's also the end part of that a few lines down...

Copy link
Author

Choose a reason for hiding this comment

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

I thought I had found all of these and removed them but clearly missed a couple, thanks for the catch!

@jchodera
Copy link
Member

Perhaps we could use information from the ForceField itself, since we don't need the geometry here? That way, we could make sure this works for any polymer rather than just the residues for which we have PDBs generated here.

@pgrinaway
Copy link
Member

Perhaps we could use information from the ForceField itself, since we don't need the geometry here? That way, we could make sure this works for any polymer rather than just the residues for which we have PDBs generated here.

+1

@jchodera
Copy link
Member

We may have to dig into the private ForceField API a bit for that, but we can expose things via the public API as we figure out what the best way to do that is.

Note that we want to be using openmm-dev if we aren't already since this will have the necessary ForceField changes.

@juliebehr
Copy link
Author

Yes, I had intended to do that as well! This was just simpler initially, and is directly taken from pdbfixer.

@pgrinaway
Copy link
Member

Do you think we can enable the Python 3 builds and tests now? It's a change to .travis.yml so that this part is:

    - python=3.4  CONDA_PY=34
    - python=3.5  CONDA_PY=35

(note that I've changed the 3.3 to 3.4 and 3.4 to 3.5.)

Thanks!

@pgrinaway
Copy link
Member

This failure is a result of the OpenEye licensing model, I think. We need the key to be present during the build, but it's only accessible from branches in the master repository.

@pgrinaway
Copy link
Member

What are we doing with this PR now?

@jchodera
Copy link
Member

Do you think we can enable the Python 3 builds and tests now?

Go for it, but note the OEChem tools have a bug for python 3.5.

@pgrinaway
Copy link
Member

Interesting. I am running Python 3.5.1 as my primary interpreter on my laptop and I have no issue with OpenEye. Maybe it's a linux issue?

@jchodera
Copy link
Member

Yikes---good question. I guess we eventually want to include pKa capabilities as well.

For now, is there a way we can pick either of the singly-protonated HIS protomers for now? I think these are HID and HIE? That would solve 90% of the problem of not knowing which tautomer is the optimal one.

@pgrinaway
Copy link
Member

For now, is there a way we can pick either of the singly-protonated HIS protomers for now? I think these are HID and HIE? That would solve 90% of the problem of not knowing which tautomer is the optimal one.

So we could just (for now) uniformly draw one of them?

@jchodera
Copy link
Member

So we could just (for now) uniformly draw one of them?

Yes! We just want to choose the residue first (HIS) and then choose one of the tautomers uniformly (HID/HIE).

@jchodera
Copy link
Member

We do want to make sure both tautomers (HID/HIE) count in the same HIS bin for free energy estimates, though---they shouldn't be counted separately as different species in the target weight \pi_j or the free energy estimates \zeta_j.

hasMatch[i] = False
return False

def _match_template_and_res_name(topology, residue_map, forcefield):
Copy link
Author

Choose a reason for hiding this comment

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

This test is still currently broken -- I was trying to confirm that the new residue has been built correctly by using forcefield to find a matching template and then asserting that they have the same name (i.e. the residue has a match and also it's the correct template) by copying the first lines of createSystem() in forcefield.py where the template matches are found. Something about this method isn't working, as it will find the signature in _templateSignatures (Line 147) but then does not match the atoms (Line 149: matches = None). There are then no generators in forcefield._templateGenerators which is why the loop at Line 156 does not lead to a template or Exception.

I don't think this has to do with the topologies being built incorrectly such that a template match cannot be found, because I can successfully run ff.createSystem(new_topology).

(Note that the excessive assert and prints are just from me trying to pinpoint where the method was going wrong).

Copy link
Member

Choose a reason for hiding this comment

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

Note that the template signatures matching onlymeans that you have the same composition of atoms; you can still subsequently fail matching in the _findAtomMatches() stage because the bonds don't match up.

@jchodera
Copy link
Member

Is the goal of the tests just to use the ForceField template matching machinery to read off the list of residues you have after the topology proposal? If so, that's (1) really clever, and (2) a feature we can probably even incorporate into the ForceField class itself so that you can feed in a Topology object and get back a list of matching forcefield residue templates or template names. I could see that being widely useful.

@juliebehr
Copy link
Author

Yes; unfortunately not as successfully as I'd hoped.

@pgrinaway
Copy link
Member

I'm getting a weird error when I try to run this:

Traceback (most recent call last):
  File "/Users/grinawap/PycharmProjects/perses_juile/perses/tests/test_topology_proposal.py", line 437, in <module>
    test_mutate_from_every_amino_to_every_other()
  File "/Users/grinawap/PycharmProjects/perses_juile/perses/tests/test_topology_proposal.py", line 339, in test_mutate_from_every_amino_to_every_other
    modeller = pm_top_engine._add_new_atoms(modeller, missing_atoms, residue_map)
  File "/Users/grinawap/PycharmProjects/perses_juile/perses/rjmc/topology_proposal.py", line 675, in _add_new_atoms
    modeller.topology.addBond(new_bonded_0, new_bonded_1)
  File "/Users/grinawap/anaconda/lib/python3.5/site-packages/simtk/openmm/app/topology.py", line 168, in addBond
    self._bonds.append((atom1, atom2))
AttributeError: 'filter' object has no attribute 'append'

It's weird, because it's happening inside app.Topology.addBond, where the _bonds attribute seems to be a filter instead of a list.

@pgrinaway
Copy link
Member

For the record, the iteration index k is at 0, so this is the first iteration of the _add_new_atoms

atom.residue._atoms.remove(atom)
for bond in modeller.topology._bonds:
if atom in bond:
modeller.topology._bonds = filter(lambda a: a != bond, modeller.topology._bonds)
Copy link
Member

Choose a reason for hiding this comment

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

what about here? the list becomes a filter?

@jchodera
Copy link
Member

See openmm/openmm#1359 for new function added to help support your tests.

This would be used as:

templates = forcefield.getMatchingTemplates(new_topology)
assert(template[index].name == expected_template_name)

@juliebehr
Copy link
Author

Awesome!

@jchodera
Copy link
Member

openmm/openmm#1359 was merged, so you should be able to use it for your test @juliebehr

@juliebehr
Copy link
Author

@jchodera great, updating openmm and adding it to the test now!

@jchodera
Copy link
Member

jchodera commented Jan 22, 2016 via email

@jchodera
Copy link
Member

jchodera commented Jan 22, 2016 via email

@jchodera
Copy link
Member

Fixed!

@juliebehr
Copy link
Author

As far as I can tell (unless there are additional things that should be tested) this PR is now complete.

@pgrinaway
Copy link
Member

Runs fine on my local computer.

@juliebehr juliebehr changed the title [WIP] initial PolymerProposalEngine Initial PolymerProposalEngine Jan 22, 2016
juliebehr pushed a commit that referenced this pull request Jan 22, 2016
Initial PolymerProposalEngine
@juliebehr juliebehr merged commit 539ee98 into choderalab:master Jan 22, 2016
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.

None yet

3 participants