-
Notifications
You must be signed in to change notification settings - Fork 144
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
[ENH] Added IDK² and s-IDK² Anomaly Detector To Aeon #2465
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to
|
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 just had a very brief look at the code because I am short on time. I will have another look next year after the Christmas break. Meanwhile, the code quality can be improved and tests need to be added.
- The tests are missing! Please add reasonable tests for the new algorithm. How do you make sure that it produces the same results as the original implementation?
- The code mixes standard python (e.g.
random
,range
, ...) with numpy (e.g.np.random
,np.arange
, ...). Please mostly rely on numpy for better code quality and performance. - Why are most methods in capital letters and start with dunders (
__
)? If this is a reference to the original implementation, please add the original name as a comment and use our coding standard.
Thank you for taking the time and contributing to aeon!
aeon/anomaly_detection/_idk.py
Outdated
t : int, default=100 | ||
Number of iterations (time steps) for random sampling to construct | ||
feature maps. |
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.
t
is not very descriptive. I would call it max_iter
similar on how sklearn does it, e.g. for 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.
I utilized 't' as it was implemented this way in the original code
Hello, I recently added a test case, but it seems to be causing errors. The test case runs successfully in my local environment, so I'm unsure why the issue arises. Could you assist me in identifying the cause? |
closes #2114