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

Optimisation of flexible side chains #73

Merged
merged 61 commits into from
Aug 21, 2019
Merged

Optimisation of flexible side chains #73

merged 61 commits into from
Aug 21, 2019

Conversation

RMeli
Copy link
Member

@RMeli RMeli commented Jul 29, 2019

GSoC 2019

Goal

Implement optimisation of flexible side chains using the CNN scoring function.

Tasks:

  • Split ligand and receptor movable atoms in the correct channels
  • Get combined gradient of the ligand and receptor flexible atoms

Changes

  • Automatically freeze receptor (--cnn_freeze_receptor) when minimising with CNN scoring function (--cnn_scoring --minimize)
  • Split ligand and receptor atoms in the correct channels
    • CNNScorer::ligand_coords and CNNScorer::ligand_smtypes attributes to store ligand coordinates and smina atom types
    • CNNScorer::receptor_coords and CNNScorer::receptor_smtypes attributes to store receptor coordinates and smina atom types
    • CNNScorer::setReceptor and CNNScorer::setLigand private members to automatically fill CNNScorer::ligand_coords, CNNScorer::ligand_smtypes, CNNScorer::receptor_coords and and CNNScorer::receptor_smtypes from model::atoms, model::coorsd and model::grid_atoms
    • New and unified MolGRidDayaLayer::setReceptor and MolGRidDayaLayer::setLigand API, accepting ligand/receptor coordinates and smina atom types
  • Get combined ligand and flexible residues gradient
    • New CNNScorer::setGradient private member correctly extracting ligand (and flexible residues) gradient from MolGridDataLayer.
  • Print the list of flexible residues after gnina banner
    • New FlexInfo::printFlex function

Copy link
Contributor

@dkoes dkoes left a comment

Choose a reason for hiding this comment

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

What I'm envisioning is a clean break between molgridlayer and gnina where gnina says to molgrid "these are the receptor atoms", "these are the ligand atoms" and molgrid doesn't need to worry if what's flexible or not - all molgrid needs are the types and coordinates. This means you need to splice together rigid and flexible atoms (and then separate them) but seems like a much cleaner interface to me.

If a class variable is used to assemble/dissassemble the atoms, this should be relatively efficient (no need to allocate/deallocate memory, just copying a few numbers).

gninasrc/lib/cnn_scorer.cpp Outdated Show resolved Hide resolved
@RMeli
Copy link
Member Author

RMeli commented Aug 2, 2019

@dkoes, thanks for the code review and the comments.

This means you need to splice together rigid and flexible atoms (and then separate them) but seems like a much cleaner interface to me.

Isn't this similar to what I'm doing (after @Jsunseri code review)?

CNNScorer now has the following new attributes:

  • atomv ligand_atoms, receptor_atoms;
  • vecv ligand_coords, receptor_coords;
  • std::size_t num_flex_atoms;

which explicitly differentiate between ligand and receptor atoms (both flexible and not; flexible ones are just put at the beginning and tracked with CNNScorer::num_flex_atoms) and are passed to MolGridDataLayer.

The functions setLigand and setReceptor are used to to extract rigid and flexible atoms (of both ligand and receptor) from the model (m.get_movable_atoms(), m.get_fixed_atoms()) and populate the new attributes. The same functions are also used to update the flexible atoms coordinates on subsequent calls, but allocation should be performed only once (and the atomv are untouched after the first call).

I think the current implementation is very similar to what you describe (since there are almost no changes to MolGridDataLater apart from a API change where receptor coordinates are passed explicitly) but if my understanding is wrong and you have something different in mind could you provide some more details? Thanks!

gninasrc/lib/cnn_scorer.cpp Outdated Show resolved Hide resolved
@RMeli
Copy link
Member Author

RMeli commented Aug 4, 2019

A possible improvement I see at the moment is that CNNScorer::setReceptor and CNNScorer::setLigand are also used to update the coordinates from model->coords to the internal CNNScorer->ligand_coords and CNNScorer->receptor_coords (for subsequent calls of the CNNScorer::score function). Would it be cleaner to pass the model to the CNNScorer constructor, call CNNScorer::setReceptor and CNNScorer::setLigand only once and update the coordinates via a different utility function (private member of CNNScorer)? Or the same CNNScorer needs to be used with different models?

@RMeli
Copy link
Member Author

RMeli commented Aug 5, 2019

Updated PR description to reflect latest changes.

Copy link
Contributor

@dkoes dkoes left a comment

Choose a reason for hiding this comment

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

At first glance, isn't obvious to me why you aren't clearing a vector

gninasrc/lib/cnn_scorer.cpp Outdated Show resolved Hide resolved
gninasrc/lib/cnn_scorer.cpp Show resolved Hide resolved
@dkoes dkoes merged commit 86c3c10 into gnina:master Aug 21, 2019
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