LR_Detection #52
LR_Detection #52
Conversation
|
@alexstihi I turned your branch into a PR, so that we have a place to add comments and discuss |
This implementation now uses the dev guidelines for handling pre-trained models and parameters, available at: https://github.com/mobilise-d/gaitlink/blob/main/examples/dev_guides/_01_pretrained_models.py. Also updated the reference data extractor to use reference_parameters_.wb_list and reference_parameters_.ic_list, instead of walking_bouts and initial_contacts.
| from gaitlink.lr_detection import UllrichLRDetection | ||
|
|
||
|
|
||
| class UllrichLROptimizer(): |
There was a problem hiding this comment.
I don't think we need this. You are basically just wrapping GridSearchCV
|
|
||
|
|
||
|
|
||
| class LROptiPipeline(OptimizablePipeline): |
There was a problem hiding this comment.
This is cool to have! Makes it easier to do optimization right away. I am not sure what the best place in the codebase and the best name for the class is, but let's leave it here for now.
| # return y | ||
|
|
||
|
|
||
| def find_extrema_in_radius(data: np.ndarray, |
There was a problem hiding this comment.
These two funcitons are from gaitmap right? Can you Note that somewhere? :) Ideally with the link to the original implementation
There was a problem hiding this comment.
These are from Martin's code, not gaitmap, however they are deprecated.
gaitlink/lr_detection/base.py
Outdated
| # TODO: Note that these models will need to be imported according to the groups of individuals they were trained on: HC, PD, MS, etc. | ||
|
|
||
| @base_lr_detector_docfiller | ||
| class PretrainedModel(StrEnum): |
There was a problem hiding this comment.
Let's Move this to the lr_detect_ML file, as it is specifc for this.
There was a problem hiding this comment.
This is the deprecated version, and it will be removed in the future. I have updated the main UllrichLRDetection class to contain an inner class (as per the dev guidelines) holding the pre-trained model configurations, i.e. filtering parameters, pretrained sklearn models and sklearn min-max scalers. I don't think there will be a need for another PretrainedModel class...maybe?
gaitlink/lr_detection/base.py
Outdated
| model_path = base_dir / 'msproject_ms_model.gz' | ||
| return joblib.load(model_path) | ||
|
|
||
| # Note, these are not pretrained models, they are just some hyperparameters. They are here for convenience, since they were not available. |
There was a problem hiding this comment.
Do we really need this? Let's only support the actual pre-trained models that we actually have
There was a problem hiding this comment.
Agreed, see the PredefinedParameters inner class for UllrichLRDetection.
| raise TypeError(f"Unknown model type {type(model)}. The model must be of type {ClassifierMixin}") | ||
|
|
||
| @base_lr_detector_docfiller | ||
| def detect(self, data: list[pd.DataFrame], ic_list: list[pd.DataFrame], sampling_rate_hz: float = 100): |
There was a problem hiding this comment.
Detect should have the same call signature as McCamley. So no list of dataframes, but just a asingle dataframe. Training is called on multiple datapoints, but inference always on one.
|
|
||
| self.ic_lr.append(prediction_per_gs) | ||
|
|
||
| # ALEX: It might be more elegant to only return the value of the prediction and nothing else: i.e. do not store anything internally (data, ic_list, processed_data, predictions) |
There was a problem hiding this comment.
We want to follow the API contract that can be fullfilled by all algorithms.
| return self | ||
|
|
||
|
|
||
| @base_lr_detector_docfiller |
There was a problem hiding this comment.
If you apply the docfiller, make use of the docfiller :)
|
|
||
| # Fit the scaler if fit_scaler is True, otherwise just transform the data | ||
| if fit_scaler: | ||
| feature_df = pd.DataFrame(self.scaler.fit_transform(feature_df.values), columns=feature_df.columns, index=feature_df.index) |
There was a problem hiding this comment.
If you fit the scaler, you should fit it on the entire data not per GS or am I wrong? Is this intended that you normalize each GS?
There was a problem hiding this comment.
Correct, somehow I missed this. I will update this in the next push.
|
|
||
| def __init__(self, | ||
| model: ClassifierMixin = None, | ||
| scaler: MinMaxScaler = None, |
There was a problem hiding this comment.
Ideally set the actual default instead of None
|
Started extracting individual Algos into separate PRs. First one #100 Will leave this one open, until others are merged |
|
Second one #106 |
|
Both new PRs merged. Closing this one as well |
No description provided.