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

Enhance flexible definition in config files / defaults.yaml #969

Merged
merged 45 commits into from
Sep 12, 2024
Merged

Conversation

amjjbonvin
Copy link
Member

@amjjbonvin amjjbonvin commented Aug 7, 2024

You are about to submit a new Pull Request. Before continuing make sure you read the contributing guidelines and that you comply with the following criteria:

  • You have sticked to Python. Please talk to us before adding other programming languages to HADDOCK3
  • Your PR is about CNS
  • Your code is well documented: proper docstrings and explanatory comments for those tricky parts
  • You structured the code into small functions as much as possible. You can use classes if there is a (state) purpose
  • Your code follows our coding style
  • You wrote tests for the new code
  • tox tests pass. Run tox command inside the repository folder
  • -test.cfg examples execute without errors. Inside examples/ run python run_tests.py -b
  • PR does not add any dependencies, unless permission granted by the HADDOCK team
  • PR does not break licensing
  • Your PR is about writing documentation for already existing code 🔥
  • Your PR is about writing tests for already existing code :godmode:

Simplified the fully flexible segments definitions, removing the per-molecule definition.
This removes a lot of parameters from the defaults.yaml files.
Also cleaned up the related CNS scripts, removing unused parameters.

Closes #919

@amjjbonvin amjjbonvin added enhancement Enhancing an existing feature of adding a new one CNS Improvements in the CNS engine m|emref emref module m|flexref flexref module m|mdref mdref module labels Aug 7, 2024
@amjjbonvin amjjbonvin self-assigned this Aug 7, 2024
@VGPReys VGPReys changed the title Solves issue #919 Enhance flexible definition in config files / defaults.yaml Aug 9, 2024
Copy link
Contributor

@VGPReys VGPReys left a comment

Choose a reason for hiding this comment

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

Overall great improvements in the readability of the default parameter file.

  • Defining the chain to be flexible is probably more intuitive for the user.
  • There was an already implemented check that all 3 fle_sta_Y, fle_end_Y and fle_seg_Y are defined, so that's cool.

Nevertheless, I do have some comments related to this new implementation:

  • I feel it weird that only the nfle parameter is modified but not nseg, which, from what I understand for now, follows the same semantic.
  • Also, the new parameter fle_seg_X is strange, because on one side seg is used to defined a segment ID, and in nseg to define number of semi-flexible segments. Can't we use fle_chain_X instead ?
  • Do we really need to keep the nfle parameter while we could also count the number of fle_sta_X that should also match the same number ?
  • There is no error when nfle value no not match the number of fle_sta_X definitions. e.g.:
nfle = 1
fle_sta_1 = 1
fle_end_1 = 5
fle_seg_1 = "B"
fle_sta_2 = 6
fle_end_2 = 11
fle_seg_2 = "B"

so I guess computing nfle on-the-fly could be an interesting solution.

  • In some part of the haddock3 parameters, we use moleculesID and some other chainID/segmentID. I believe this might get very confusing to some extend. I don't know what is best. Do we still want to use molecule IDs or do we want to talk about chain IDs ? Uniforming this could also help the users.

@amjjbonvin
Copy link
Member Author

amjjbonvin commented Aug 9, 2024

There is a reason why only nfle has been changed and not nsegX: By having molecule-specific nsegX parameters we can control the flexibility per molecule with the following options:

  • -1 -> automatic definition of semi-flexible segments
  • 0 -> molecule treated as rigid
  • >1 -> manual definition of semi-flexible segments.

About computing nlfe on the fly, this should be then implemented at the python level in the module and the corresponding parameters passed to CNS. You are welcome to try :-)

As for the seg vs chain naming, formally the chainID comes after the residue name, while the segID comes at the end and this is what CNS used internally. This is also the reason why we should may-be stick to moleculeID in the documentation to cover both chainID and segID.

@amjjbonvin
Copy link
Member Author

And we are speaking here about full flexibility of defined segment, which means flexible from the start of the simulated annealing process. This is different than the semi-flexible segments that are only treated as flexible in the last two cooling phases, first only side-chains and then side-chains and backbone.

@rvhonorato rvhonorato requested review from mgiulini and removed request for rvhonorato August 12, 2024 09:21
@rvhonorato rvhonorato self-assigned this Aug 12, 2024
@rvhonorato
Copy link
Member

I've added the missing integrations tests to account for this parameters, test_mdref, test_emref, test_flexref

mgiulini
mgiulini previously approved these changes Aug 13, 2024
integration_tests/test_emref.py Outdated Show resolved Hide resolved
integration_tests/test_flexref.py Outdated Show resolved Hide resolved
integration_tests/test_mdref.py Outdated Show resolved Hide resolved
Changes/additions made by Rodrigo will be in a seperate branch/pull request
VGPReys
VGPReys previously approved these changes Sep 10, 2024
VGPReys
VGPReys previously approved these changes Sep 12, 2024
Copy link
Contributor

@AnnaKravchenko AnnaKravchenko left a comment

Choose a reason for hiding this comment

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

From now on it will be much easier to define a fully flexible segment, amazing!
I’ve added some suggestions for extra clarity, but they are rather minor

src/haddock/modules/refinement/mdref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/mdref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/mdref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/mdref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/flexref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/emref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/emref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/emref/defaults.yaml Outdated Show resolved Hide resolved
src/haddock/modules/refinement/emref/defaults.yaml Outdated Show resolved Hide resolved
@amjjbonvin amjjbonvin merged commit 513ccea into main Sep 12, 2024
4 checks passed
@amjjbonvin amjjbonvin deleted the flexseg branch September 12, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CNS Improvements in the CNS engine enhancement Enhancing an existing feature of adding a new one m|emref emref module m|flexref flexref module m|mdref mdref module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flexible segments definition
5 participants