-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add option to select all clusters in seletopclusts
#286
Conversation
@@ -14,13 +15,8 @@ class HaddockModule(BaseHaddockModule): | |||
|
|||
name = RECIPE_PATH.name | |||
|
|||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Black lint is killing us. Much better before, also contributions signatures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*killing you, the linting is passing 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, because is not a matter of lint, is a matter of style :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mine or the lack of yours? 🔥
@@ -34,20 +30,32 @@ def _run(self): | |||
|
|||
# how many models should we output? | |||
models = [] | |||
for target_ranking in self.params['top_cluster']: | |||
if self.params['top_models'] == 'all': | |||
if not self.params["top_cluster"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happen if the user inputs top_cluster = 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question! let me address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an extra check, thanks!
@@ -1,2 +1,2 @@ | |||
top_cluster = [1, 2, 3, 4] | |||
top_models = 4 | |||
top_cluster = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want the defaults to provide all models? Or should the defaults have a more natural value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all models, but models for all clusters - this is a different thing. There is no warranty that the correct solutions are in the top4... and further refinement could well change the ranking at the end.
@@ -26,6 +26,15 @@ def confirm_installation(cls): | |||
|
|||
def _run(self): | |||
"""Execute the module's protocol.""" | |||
|
|||
if self.params["top_models"] <= 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, fine for now but I need to find a better way around this. Also to make sure the right types are passed in the user config files.
This pr closes #282 by implementing a different logic to loop over the target cluster rankings