Add refactored LGBM model to experimental emulators #399
Conversation
Coverage report
Click 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 #399 +/- ##
==========================================
- Coverage 91.95% 90.13% -1.82%
==========================================
Files 84 91 +7
Lines 4721 5678 +957
==========================================
+ Hits 4341 5118 +777
- Misses 380 560 +180 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| self.n_features_in_ = x.shape[1] | ||
|
|
||
| x, y = check_X_y( |
There was a problem hiding this comment.
What does chech_X_y do? I feel like any data checks and conversions we want to do should be in the InputTypeMixin (or something similar)?
There was a problem hiding this comment.
This checks X and y have the same length and also that X is 2D and y 1D, however there are also other checks you can include that in the old Autoemulate have been set up differently depending on the emulator class e.g. for SVM:
X, y = check_X_y(
X,
y,
multi_output=self._more_tags()["multioutput"],
y_numeric=True,
ensure_min_samples=2,
)
and for polynomials:
X, y = check_X_y(X, y, multi_output=True, y_numeric=True, dtype=np.float64)
Perhaps for now it's best to leave these checks in the fit methods in, then to refactor into the InputTypeMixin later if several of the models we include in experimental are all doing basically the same check or we disagree with some of the differences and think that a simple dimensionality check is sufficient?
There was a problem hiding this comment.
Agree with both the above (#399 (comment)) and keeping these checks to be refactored in a new issue for adding a class for validation/checks seems like a good option.
| def model_name(self): | ||
| return self.__class__.__name__ | ||
|
|
||
| def _more_tags(self): |
There was a problem hiding this comment.
Can we remove this method and just not call it above?
There was a problem hiding this comment.
Yes, this seems like a weird way to do it - multi_output defaults to False so in this case can remove completely
| def predict(self, x: InputLike) -> OutputLike: | ||
| """Predicts the output of the emulator for a given input.""" | ||
| x = check_array(x) | ||
| check_is_fitted(self, "is_fitted_") |
There was a problem hiding this comment.
If using this check function is dependent on this object inheriting the the sklearn base objects I'd be in favour of not doing the inheritance and just getting rid of this (and replacing it with our own check if we think that's necessary)
…re proper output format
…ability" This reverts commit 83a239f.
… and simplify Tuner model instantiation Co-authored-by: Radka Jersakova <[email protected]> Co-authored-by: Sam Greenbury <[email protected]>
|
Final todos:
|
get_tune_configmethodscipy.statsand usenpinstead_convert_to_numpyfunction to utils, used by the fit method