-
Notifications
You must be signed in to change notification settings - Fork 122
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
[MNT, DOC] Accelerating deep testing #1904
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mind if you do it here. The dummy option is not a good addition IMO.
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.
will add the changes then
aeon/networks/_ae_resnet.py
Outdated
@@ -138,36 +138,43 @@ def build_network(self, input_shape, **kwargs): | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise an actual error with a message instead of asserting
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
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.
It would be better as a ValueError
IMO
…deep-cltr-testing
Fix #1761