Conversation
itellaetxe
left a comment
There was a problem hiding this comment.
LGTM. Minor stuff:
- Argparser
nargscomments - Inheritance for
...FoldMetrics - Docstrings should be in the same format, they are not consistent.
| self.parser.add_argument( | ||
| "-m", | ||
| "--model", | ||
| nargs="*", |
There was a problem hiding this comment.
why nargs="*"? (nargs doc)
Can you specify more than one model? Or you just want to put the specified model string into a list? E.g.-> you specify "ridge" and it automatically gets put into ["ridge"]
If you only want one or zero arguments, you should instead change this to nargs="?"
There was a problem hiding this comment.
So this is inherited from previous code above, see model_age. When building the models the user can choose to use one of the availble models (only one). However, they can specify several other arguments to set in the model. Example: -m linear_reg fit_intercept=False normalize=True. As the user can input no arguments (default uses) or one argument (model type that will use the default settings for tha model type) or muliple arguments which woul require then *. If it is more appropriate to use ? we should change it above too.
There was a problem hiding this comment.
Okay seems reasonable! nargs="*" is good for our use case, then. 👍
| self.parser.add_argument( | ||
| "-s", | ||
| "--scaler", | ||
| nargs="*", |
There was a problem hiding this comment.
See comment above about nargs. Same applies to --scaler argument here.
There was a problem hiding this comment.
I have replied in the commment above and its the same case it can have 0, 1 or muliple arguments.
| raise ValueError('task_type must be either "regression" or "classification"') | ||
| else: | ||
| self.task_type = task_type | ||
| self.train_metrics: List[Union[RegressionFoldMetrics, ClassificationFoldMetrics]] = [] |
There was a problem hiding this comment.
To avoid this List and Union thing, use class inheritance.
Making RegressionFoldMetrics and ClassificationFoldMetrics children of the same abstract parent class e.g. FoldMetrics would be easier. This way you just have to check if the parent class of the input is of type FoldMetrics.
There was a problem hiding this comment.
I agree this has been changed.
| self.test_metrics.append(fold_test) | ||
|
|
||
| def _calculate_summary(self, metrics_list: List[Union[RegressionFoldMetrics, ClassificationFoldMetrics]]) -> Dict[str, Dict[str, float]]: | ||
| # TODO - Automatically infer instead of using Union to have both types |
There was a problem hiding this comment.
See comment above about inheritance with FoldMetrics
| plt.close() | ||
|
|
||
| def ordering(self, mi_age, mi_discr, feature_names, system_dict, title): | ||
| """Plot in the ssame figure the mutual information for age and discrimination.""" |
There was a problem hiding this comment.
I have now updated the docstring.
| plt.close() | ||
|
|
||
| def multiple_metrics_vs_num_features(self, metrics_age, metrics_discrimination, title): | ||
|
|
There was a problem hiding this comment.
This has now been added
33c2235 to
5b53958
Compare
itellaetxe
left a comment
There was a problem hiding this comment.
LGTM. Merging. Ty for the work @JGarciaCondado 🤝
| self.parser.add_argument( | ||
| "-m", | ||
| "--model", | ||
| nargs="*", |
There was a problem hiding this comment.
Okay seems reasonable! nargs="*" is good for our use case, then. 👍
Summary
We have added two new methods to the ageml processing pipeline. The new commands are:
model_feature_influenceage_model_vs_logistic_regressionDescription of commands
model_feature_influence:Output:
age_model_vs_logistic_regressionThis pipeline gives you an idea of the benefit of computing the deltas to classify clinical groups, compared to just using the features.
Output: