Skip to content
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

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

Ramana-Raja
Copy link

closes #2114

@aeon-actions-bot
Copy link
Contributor

Thank you for contributing to aeon

I did not find any labels to add based on the title. Please add the [ENH], [MNT], [BUG], [DOC], [REF], [DEP] and/or [GOV] tags to your pull requests titles. For now you can add the labels manually.
I have added the following labels to this PR based on the changes made: [ $\color{#6F6E8D}{\textsf{anomaly detection}}$ ]. Feel free to change these if they do not properly represent the PR.

The Checks tab will show the status of our automated tests. You can click on individual test runs in the tab or "Details" in the panel below to see more information if there is a failure.

If our pre-commit code quality check fails, any trivial fixes will automatically be pushed to your PR unless it is a draft.

Don't hesitate to ask questions on the aeon Slack channel if you have any.

PR CI actions

These checkboxes will add labels to enable/disable CI functionality for this PR. This may not take effect immediately, and a new commit may be required to run the new configuration.

  • Run pre-commit checks for all files
  • Run mypy typecheck tests
  • Run all pytest tests and configurations
  • Run all notebook example tests
  • Run numba-disabled codecov tests
  • Stop automatic pre-commit fixes (always disabled for drafts)
  • Disable numba cache loading
  • Push an empty commit to re-run CI checks

@Ramana-Raja Ramana-Raja changed the title Added IDK² and s-IDK² Anomaly Detector To Aeon [ENH]Added IDK² and s-IDK² Anomaly Detector To Aeon Dec 19, 2024
@Ramana-Raja Ramana-Raja changed the title [ENH]Added IDK² and s-IDK² Anomaly Detector To Aeon [ENH] Added IDK² and s-IDK² Anomaly Detector To Aeon Dec 19, 2024
@MatthewMiddlehurst MatthewMiddlehurst added the enhancement New feature, improvement request or other non-bug code enhancement label Dec 19, 2024
Copy link
Member

@SebastianSchmidl SebastianSchmidl left a 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 Show resolved Hide resolved
Comment on lines 37 to 39
t : int, default=100
Number of iterations (time steps) for random sampling to construct
feature maps.
Copy link
Member

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

Copy link
Author

@Ramana-Raja Ramana-Raja Dec 20, 2024

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

aeon/anomaly_detection/_idk.py Outdated Show resolved Hide resolved
@Ramana-Raja
Copy link
Author

Ramana-Raja commented Jan 5, 2025

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!

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
anomaly detection Anomaly detection package enhancement New feature, improvement request or other non-bug code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Implement (s-)IDK² anomaly detector
3 participants