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

Remove input layer in topoaa module. #300

Merged
merged 3 commits into from
Jan 31, 2022
Merged

Remove input layer in topoaa module. #300

merged 3 commits into from
Jan 31, 2022

Conversation

joaomcteixeira
Copy link
Member

@amjjbonvin one of the things we discussed in today's IVRESSE meeting was the possibility to remove the input layer from the topoaa module such that we have only mol1, mol2, etc, directly in the top layer of the config file. As you can see from the changes in this PR, removing the input layer adds no burden to the Python API (see changes in the topoaa/__init__.py file. I have these changes already prepared for the #274 if you accept them.

@sverhoeven

@joaomcteixeira joaomcteixeira added the m|topoaa topoaa module label Jan 28, 2022
@joaomcteixeira joaomcteixeira self-assigned this Jan 28, 2022
@joaomcteixeira
Copy link
Member Author

Another thing we could consider is to remove the mol beyond 1. These are basically the same with the exception that prot_segid is a cycle over the alphabet. Can't we remove mol2 to mol20? and make that an expandable engine as well? The user could define the parameters in the config file for mol3 if needed without mol3 being defined in the topoaa/defaults.cfg file.

@amjjbonvin
Copy link
Member

We could indeed make the mol parameters expandable. I would however keep the prot_segid ones, since now a user does not need per se to define those. If we remove them then this must be defined in the user config file. Or the python code must automatically define those incrementing the chainIDs in case they are not defined by the user.

@@ -10,8 +10,7 @@ limit = true
# tolerance in percentage
tolerance = 0

[input]
[input.mol1]
[mol1]
Copy link
Member

Choose a reason for hiding this comment

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

So no need for topoaa in the param name here? Will be confusing to users since our example config files do use the topoaa.molX naming. To me the default.cfg file of a module shows me all the possible parameters and the way to define those. I think we already discussed that before since we have the same inconsistency before these changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a different thing. In the defaults.cfg we use [mol1] and not [topoaa.mol1] because the default.cfg itself is the topoaa module, while the user config has several modules so we need to define the [topoaa] layer. Keep in mind that the user will never access the defaults.cfg per se. We will always provide a script or an API that translates the defaults.cfg to what the user needs. I see in #274 I've create an haddock3-cfg CLI for that purpose. Actually, after #274 is merged the concern you raised in your comment get's solved because we move from the defaults from cfg to yml.

@joaomcteixeira
Copy link
Member Author

Accepting. Good addition 👍

@joaomcteixeira joaomcteixeira merged commit 1ee6165 into main Jan 31, 2022
@joaomcteixeira joaomcteixeira deleted the topoaainput branch January 31, 2022 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
m|topoaa topoaa module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants