Add linear regression #15
Conversation
|
There was a problem hiding this comment.
Looks good @SarahAlidoost, just a few minor comment.
One think that I have thought of right now while looking at the code is data normalization, which I think is not implemented but maybe should be included as part of the data preparation (maybe in the pipeline)?
| @dataclass | ||
| class Estimator: | ||
| name: str | ||
| func: callable |
There was a problem hiding this comment.
If I understand correctly, this is actually assigned an estimator instance (something that implements estimator.fit() and estimator.predict()) - so not a callable function, right?
| "mean_absolute_error": sm.mean_absolute_error, | ||
| } | ||
|
|
||
| def __init__(self, estimator: str | Estimator, kwargs: dict = None) -> None: |
There was a problem hiding this comment.
Maybe instead of kwargs either use **kwargs (i.e. with unpacking) or use a more descriptive name (maybe params?)?
Also, I am not 100% sure, but if a parameter has None as default, this should be made explicit in the type hint:
| def __init__(self, estimator: str | Estimator, kwargs: dict = None) -> None: | |
| def __init__(self, estimator: str | Estimator, kwargs: dict | None = None) -> None: |
| if kwargs: | ||
| for key, value in kwargs.items(): | ||
| setattr(self.estimator.func, key, value) |
There was a problem hiding this comment.
Estimator objects in scikit-learn should have a set_params method (see link), so maybe one could do:
| if kwargs: | |
| for key, value in kwargs.items(): | |
| setattr(self.estimator.func, key, value) | |
| self.estimator.func.set_params(**kwargs) |
closes #2
closes #19