Fill out a few function docstrings #196
Conversation
|
|
||
| Returns | ||
| ------- | ||
| Pandas.Dataframe |
| String indicating model to be used, e.g. "RandomForestClassifier". | ||
| If None, `model` must not be. Defaults to None. | ||
| model_options : dict, optional | ||
| Dictionary with hyperparameter values to be used in model building. |
There was a problem hiding this comment.
Describe the structure of this dictionary.
| model_options : dict, optional | ||
| Dictionary with hyperparameter values to be used in model building. | ||
| params_to_optimize : list of str, optional | ||
| List of parameters to be optimized (whose corresponding entries |
There was a problem hiding this comment.
What happens if this is None?
| cv : int, cross-validation generator or an iterable, optional | ||
| Number of folds (defaults to 3) or an iterable yielding train/test | ||
| splits. See documentation for `GridSearchCV` for details. Defaults to | ||
| None. |
There was a problem hiding this comment.
What happens when this is None?
|
@stefanv addressed your comments - want to take another look? |
|
👍 here |
|
@bnaul took me a minute to see my typo... thanks |
|
|
||
| We gratefully accept contributions of new time-series features, be they | ||
| domain-specific or general. Please follow the below guidelines in order that | ||
| your features may be successfully incorporated into the Cesium feature base. |
There was a problem hiding this comment.
Where are the below guidelines?
There was a problem hiding this comment.
@stefanv bnaul offered to fill this out - he's going to contribute to my branch.
There was a problem hiding this comment.
@bnaul still up for adding this? If not, I'd like to sit down for a few minutes together to get a better sense of what this needs to be.
| If None, `model` must not be. Defaults to None. | ||
| model_options : dict, optional | ||
| Dictionary with hyperparameter values to be used in model building. | ||
| Keys are parameter names, values are the associated values. |
There was a problem hiding this comment.
How about saying what the keys are + what the associated values are and the type of structures they should be?
There was a problem hiding this comment.
@stefanv don't think I follow... "Keys are parameter names" seems pretty straightforward to me - what are you picturing beyond that?
There was a problem hiding this comment.
In the context of model hyperparameters, it seems self-explanatory
There was a problem hiding this comment.
I don't know why this is called model_options, so how about something along the lines of (please adjust as necessary):
model_parameters : dict, optional
During hyper-parameter optimization, various model parameters are adjusted to give an
optimal fit. This dictionary gives the different values that should be explored for each
parameter. E.g., `{'alpha': [1, 2], 'beta': [4, 5, 6]}` would fit models on all
6 combinations of alpha and beta and compare the resulting models'
goodness-of-fit.
| params_to_optimize : list of str, optional | ||
| List of parameters to be optimized (whose corresponding entries | ||
| in `model_options` would be a list of values to try). If None, | ||
| parameters specified in `model_options` will be passed to model |
There was a problem hiding this comment.
I think this is what should be describe above.
| model_type : str, optional | ||
| String indicating model to be used, e.g. "RandomForestClassifier". | ||
| If None, `model` must not be. Defaults to None. | ||
| model_parameters : dict, optional |
There was a problem hiding this comment.
@stefanv whatcha think of this?
There was a problem hiding this comment.
Hrm, this makes me wonder why we have a params_to_optimize dictionary. Why not just check each model_parameters entry, if it is a list, and the number of entries > 1, then hyper optimize, otherwise just use as-is.
There was a problem hiding this comment.
@stefanv params_to_optimize is a list of strings (param names). The latter is because some parameter values are lists.
There was a problem hiding this comment.
That doesn't matter if you just put the list inside of a list.
There was a problem hiding this comment.
Not sure what you mean by that... Put the list inside of a list?
There was a problem hiding this comment.
Yes, so you always have a list of parameters. If those parameters themselves are lists, then so be it.
There was a problem hiding this comment.
Correction - params_to_optimize is a dict. We need to split them at some point, so why not keep it as-is? I'll update these docs to reflect that.
There was a problem hiding this comment.
Thanks, Ari, this look good.
|
@stefanv I've updated the docstrings - please take a look when you have a moment. |
|
👍 from me |
|
oh right still waiting on feature guidelines...do those live here or will it just say "see the same file in cesium-ml/cesium"? |
|
It'll live here in CONTRIBUTING.md, which I've created. |
|
I think the |
|
@stefanv I'd love to move on this and update CONTRIBUTING.md in a separate PR |
No description provided.