[REVIEW] 018 add unfitted error pca & tests on IPCA #3272
Conversation
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
lowener
changed the title
lowener
changed the title
dantegd
added
3 - Ready for Review
| @@ -554,6 +555,7 @@ class PCA(Base): | |||
|
|
|||
| """ | |||
|
|
|||
| self._check_is_fitted('components_') | |||
There was a problem hiding this comment.
I see that in sklearn, they do sklearn.utils.validation.check_is_fitted in a similar way. Is there an advantage to having this as a method on base instead of a utility? I believe we can always add the function later as a wrapper if necessary, so probably not a major issue either way.
There was a problem hiding this comment.
The advantage of a base method would be that there is no additionnal import needed, and it would be really easy to use. But then we would have two copy of this code, for the estimators with Dask.
| @@ -736,3 +739,9 @@ class PCA(Base): | |||
| 'X_types_gpu': ['2darray', 'sparse'], | |||
| 'X_types': ['2darray', 'sparse'] | |||
| } | |||
|
|
|||
| def _check_is_fitted(self, attr): | |||
| if not hasattr(self, attr) or (getattr(self, attr) is None): | |||
There was a problem hiding this comment.
Yeah, agreed on the generalization - good suggestion from Dante
There was a problem hiding this comment.
I will do the generalization of NotFittedError in a following PR
lowener
force-pushed
the
018_add_unfitted_error_pca
branch
from
a1c56e1
to
8ddd2a9
Compare
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #3272 +/- ##
===============================================
+ Coverage 71.45% 71.48% +0.02%
===============================================
Files 205 207 +2
Lines 16594 16747 +153
===============================================
+ Hits 11858 11972 +114
- Misses 4736 4775 +39
Continue to review full report at Codecov.
|
This PR adds the
NotFittedErrorto PCA (and IncrementalPCA) so that users are warned if they usetransformorinverse_transformbefore fitting the model.I also added tests on this exception.