Skip to content

Conversation

@joaomcteixeira
Copy link
Member

@joaomcteixeira joaomcteixeira commented Nov 29, 2021

Closes #162

Parameters defined with None will NOT be written to the CNS header files. For example:

bad_param = None
bad_param = none

BUT, empty strings WILL be written to CNS files.

I prefer that empty variables are defined only by None.

Please test it, and if does not work let me know. Because it SHOULD work just with these changes.

Important: I don't know the behaviour of CNS if it expects a variable somewhere down the line that was not defined in the header.

See also #161

@joaomcteixeira joaomcteixeira self-assigned this Nov 29, 2021
@joaomcteixeira joaomcteixeira added the enhancement Improving something in the codebase label Nov 29, 2021
@joaomcteixeira joaomcteixeira added this to the v3.0.0 stable release milestone Nov 29, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Merging #163 (8b2f512) into main (81e5ae0) will decrease coverage by 0.00%.
The diff coverage is 13.88%.

❗ Current head 8b2f512 differs from pull request most recent head e8dd653. Consider uploading reports for the commit e8dd653 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #163      +/-   ##
==========================================
- Coverage   38.70%   38.70%   -0.01%     
==========================================
  Files          39       39              
  Lines        2204     2217      +13     
==========================================
+ Hits          853      858       +5     
- Misses       1351     1359       +8     
Impacted Files Coverage Δ
src/haddock/modules/topology/topoaa/__init__.py 20.45% <0.00%> (-0.73%) ⬇️
src/haddock/libs/libcns.py 16.20% <15.15%> (+1.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81e5ae0...e8dd653. Read the comment docs.

* Adds support for paths
* corrects chains, does it?
@joaomcteixeira
Copy link
Member Author

@amjjbonvin @rvhonorato I need your help and close attention in this last commit, I might have found a bug for a silenced functionality.

  1. I implemented the empty vars as we discussed, those are the changes in the libcns. (nice FP-piping there 😉)
  2. Question 1: When adding paths to the CNS files, do you want them to be absolute or relative to the CWD or where the config file is?
  3. Most importantly (the 🐛), when injecting the parameters in the CNS file there is this code, which does some special treatment of nested dictionaries to the chain keyword. However, where is that chain coming from? When I was refactoring the libcns, I came to the conclusion that chain might be what is defined in the topoaa/defaults.cfg as input, since it is the only parameter data that has that nested dictionary structure. Is that it? If yes, the previous commit corrects it, and now those parameters ARE saved in the CNS file, when before they were not. If not, please help me understand it, I might need to revert the last commit.

Try running some examples in this branch generating ONLY the topologies to see if you have an expected behaviour. There are some prints, use them or never mind them.

@amjjbonvin
Copy link
Member

amjjbonvin commented Dec 1, 2021

Another related issue:

I am testing now the manual definition of histidines, but this now requires adding all possible parameters to the default.cfg file of the topoaa module. But here is the problem: some protein might have a lot, some a few. Now for things to work we have to define many entries such as:

hisd_1 = None
hisd_2 = None
...

And if we need more variables because a protein contain more HIS, then the default file has to be edited. Could we for such nested variables allow for some flexibility, i.e. you only need to define one in the defaults.cfg file and any increased index would be accepted.

PS: The index here does not refer to the molecule, but rather to the number of HIS residue you want to define for a given molecule.

@joaomcteixeira
Copy link
Member Author

We can have a unique parameter for His and define it as a list, for example:

his = [0, 1, 0, 2, 3, 0]

each number is a histidine in the sequence and the number reflects a protonation state. That is how BioExcel Building blocks is doing.

Otherwise we can have a dictionary:

[topoaa]
[topoaa.hisd]
hisd_1=none
...

this would avoid the need to edit the defaults, since the latter just needs to have an empty [hisd] entry.

@amjjbonvin
Copy link
Member

  1. Question 1: When adding paths to the CNS files, do you want them to be absolute or relative to the CWD or where the config file is?

It could be defined with respect the MODDIR variable (this of course assumes that all data are within the user directory where haddock3 is called from

However, where is that chain coming from?

We don't have it in the default.cfg file. Instead we have per molecule the prot_segid entry

Chain might be then coming from what is defined in the PDB file itself.

@amjjbonvin
Copy link
Member

We can have a unique parameter for His and define it as a list, for example:

his = [0, 1, 0, 2, 3, 0]

each number is a histidine in the sequence and the number reflects a protonation state. That is how BioExcel Building blocks is doing.

By default all are charges and I only want to specific - per molecule - the other states when needed (either hisd or hise).
The CNS module expect those if autohis = false and numhisd or numhise > 0

> [topoaa]
> [topoaa.hisd]
> hisd_1=none

This would not work since you need to define the HIS per molecule. And each molecule can have multiple entries.
I would think that some construction as in the default.cfg file of the topoaa module should be the way to go... A

But this parameter is not showing anywhere in the defaults.cfg file.... How do I know how to define the histidine states?

@joaomcteixeira joaomcteixeira marked this pull request as ready for review December 1, 2021 13:54
Copy link
Member

@amjjbonvin amjjbonvin left a comment

Choose a reason for hiding this comment

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

I will trust you on this one and test later :-)

@joaomcteixeira
Copy link
Member Author

Done.
@amjjbonvin Now the topoaa/defaults.cfg [input] params are written to the CNS file. Before they were not.

Also, to add parameters to the [input] use:

[topoaa]
autohis=false
[topoaa.input.mol1]
whatever_new_parameter_here=false
# these parameters are not confirmed if they exist in the defaults config (for now)

@joaomcteixeira joaomcteixeira added the bug Something isn't working label Dec 1, 2021
@joaomcteixeira joaomcteixeira merged commit 282eb29 into main Dec 1, 2021
@joaomcteixeira joaomcteixeira deleted the 162 branch December 1, 2021 13:59
Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

👍🏽

raise exception(emsg.format(str(path)))


def recursive_dict_update(d, u):
Copy link
Member

Choose a reason for hiding this comment

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

oh this is neat!

return f'eval (${param}="{str(value)}"){linesep}'

elif isinstance(value, (int, float)):
return f'eval (${param}={value}){linesep}'
Copy link
Member

Choose a reason for hiding this comment

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

This will have all decimals of the float, should we do something about it to make it CNS-safe?

joaomcteixeira added a commit that referenced this pull request Dec 1, 2021
@joaomcteixeira joaomcteixeira mentioned this pull request Dec 1, 2021
joaomcteixeira added a commit that referenced this pull request Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement Improving something in the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Do not write empty parameter to CNS input

5 participants