[MNT, DOC] Accelerating deep testing#1904
Conversation
Thank you for contributing to
|
| if self.clustering_algorithm == "dummy": | ||
| self.clusterer = DummyClusterer( | ||
| n_clusters=self.n_clusters, **clustering_params_ | ||
| ) | ||
| elif self.clustering_algorithm == "kmeans": |
There was a problem hiding this comment.
Can this not just accept any BaseClusterer? Creating a useless option solely for testing is not a great way to resolve this IMO.
There was a problem hiding this comment.
i wanted to change that for accepting an estimator input instead of string, but thought it might be a lot for the PR, but to keep the PR for testing purpose this can be done, if you think its ok to get all in one PR i dont mind can do the changes here
There was a problem hiding this comment.
Don't mind if you do it here. The dummy option is not a good addition IMO.
There was a problem hiding this comment.
will add the changes then
| self._kernel_size_ = [8, 5, 3] if self.kernel_size is None else self.kernel_size | ||
|
|
||
| if isinstance(self._n_filters_, list): | ||
| assert len(self._n_filters_) == self.n_residual_blocks |
There was a problem hiding this comment.
raise an actual error with a message instead of asserting
There was a problem hiding this comment.
will do, we should raise an issue to do that all over the networks module, my code my bad ! never thought about raising a message
There was a problem hiding this comment.
It would be better as a ValueError IMO
…deep-cltr-testing
|
hi @hadifawaz1999 this is very conflicted, it might be better to start again and do it in multiple PRs? |
if am not mistaken, i dropped this and did every bullet point in a separate pr, will check what's left and close this one |
Fix #1761