-
Notifications
You must be signed in to change notification settings - Fork 27
Refactor sklearn models for experimental #415
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
Conversation
…ove unnecessary newline in SupportVectorMachines
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 80.09% 80.37% +0.28%
==========================================
Files 100 104 +4
Lines 6902 7057 +155
==========================================
+ Hits 5528 5672 +144
- Misses 1374 1385 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Adding the code from discussion with @edwardchalstrey1 and @radka-j on possible revision to the base class (relating to #322): class Emulator(ABC, InputTypeMixin):
"""
The interface containing methods on emulators that are
expected by downstream dependents. This includes:
- `AutoEmulate`
"""
@abstractmethod
def __init__(
self, x: InputLike | None = None, y: InputLike | None = None, **kwargs
): ...
@classmethod
def model_name(cls) -> str:
return cls.__name__
@abstractmethod
def _fit(self, x: InputLike, y: InputLike | None): ...
@abstractmethod
def check(self, x: InputLike, y: InputLike | None): ...
@abstractmethod
def convert(self, x: InputLike, y: InputLike | None) -> tuple[InputLike, InputLike | None]: ...
def fit(self, x, y):
"""
Fit the model to the data.
Parameters
----------
x: InputLike
Input features as numpy array, PyTorch tensor, or DataLoader.
y: OutputLike or None
Target values (not needed if x is a DataLoader).
Returns
-------
None
"""
x, y = self.convert(x, y)
self.check(x, y)
self._fit(x, y)
@abstractmethod
def predict(self, x: InputLike) -> OutputLike: ...
@staticmethod
@abstractmethod
def get_tune_config() -> TuneConfig: ... |
_convert_to_numpy Co-authored-by: Sam Greenbury <[email protected]>
fix: correct SupportVectorMachine class name in tests and other changes
| @staticmethod | ||
| def _normalize(x: TensorLike) -> tuple[TensorLike, TensorLike, TensorLike]: | ||
| x_mean = x.mean(0, keepdim=True) | ||
| x_std = x.std(0, keepdim=True) | ||
| return (x - x_mean) / x_std, x_mean, x_std | ||
|
|
||
| @staticmethod | ||
| def _denormalize( | ||
| x: TensorLike, x_mean: TensorLike, x_std: TensorLike | ||
| ) -> TensorLike: | ||
| return (x * x_std) + x_mean |
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 this here as initial functionality form preprocessing outputs. This would be good to revisit in #348 where this might be better as distinct functionality that can be combined with the model and/or updated with the API used there.
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.
Also relates to #437
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.
Note @sgreenbury I have added tests for these in #464
sgreenbury
left a comment
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.
As discussed, this looks good to merge with subsequent changes with checks/validation to follow in #422, thanks @edwardchalstrey1!
Questions:
SklearnBackend?SklearnBackend, should LightGBM use this?SklearnBackendsomething differentSklearnBackend- if not, perhaps keep them in the specific model's fit func