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

Adding PredictiveAnomalyDetection to river/anomaly #1458

Merged
merged 37 commits into from
Mar 18, 2024

Conversation

sebiwtt
Copy link
Contributor

@sebiwtt sebiwtt commented Nov 23, 2023

No description provided.

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Nov 23, 2023

@MaxHalford to be quite honest, I don't have any clue why the tests failed and I can't seem to figure it out based on the error messages in the unit-test jobs. If you have time to look into it I would really appreciate it 😄

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Nov 23, 2023

Okay, one thing I definitely need to fix is the change in some versions (idk why this happened...)

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Nov 29, 2023

I did not have time to look into the matter for a few days, but now the version numbers have reverted back to the original again, so the only two changes made with this request are in the init of river/anomaly and in the new file pad.py

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Jan 9, 2024

Hey @hoanganhngo610 do you know of a way to tweak hyperparameters on the fly? I have an idea which could boost the performance of my POC a lot...

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Jan 31, 2024

Hello there :) I just wanted to ask if someone found the time to look into this PR @hoanganhngo610, @MaxHalford?. It would mean a lot to me if I could make a contribution and if that contribution could be mentioned in my thesis. I would love to reference to the official repo when linking to this code.

@hoanganhngo610
Copy link
Contributor

Hi @sebiwtt. First of all, I am really sorry that I wasn't able to reply to this sooner. I will check out and review this PR by the next few days and will try to merge it as soon as possible when Max or another person from the development team agrees to proceed.

@hoanganhngo610 hoanganhngo610 self-requested a review February 12, 2024 17:34
@hoanganhngo610 hoanganhngo610 self-assigned this Feb 12, 2024
@hoanganhngo610
Copy link
Contributor

HI @sebiwtt. First of all, really great work on this PR, I really appreciate it!

I have made changes to small parts of the PR, mostly relating to the following points:

  • Rename variables with shorter names, re-write comments and DocString;
  • Remove the score_one_detailed function, since it is really identical to the score_one function and can easily be modified from the user's end.

In my opinion, this PR is now at a suitable shape to be merged into the main branch. I will wait for one more review, potentially from @MaxHalford or @smastelini or @AdilZouitine to see how they think

Let me know if you have any comments or suggestions on the changes that I have made. Once again, thank you so much!

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Feb 14, 2024

Hey @hoanganhngo610! Thank you very much for your review and the refinement.
If there is anything left to do, let me know :) I am happy that you regard the PR as ready for merging.

Let's wait for @MaxHalford, @smastelini or @AdilZouitine and see if they have anything to add.

Revert unnecessary changes to files not realted to the commit.
Delete setup.py file as being unnecessary.
Revert unnecessary changes to code style for unrelated file.
@hoanganhngo610
Copy link
Contributor

First of all, thank you @MaxHalford for approving the PR.

@sebiwtt Great news, we have got the green light we needed. I will be doing some final touches for this PR, before merging it. Once again, thank you so much for your contribution on this PR.

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Mar 18, 2024

Thank you so much for keeping me updated @hoanganhngo610 😁 I'm really happy to have made this contribution. Looking forward to working on topics for river in the future👍🏻

Revert unnecessary changes to code style.
Fix indentation.
Revert unnecessary changes to code style.
Refine comments within PAD.
@hoanganhngo610
Copy link
Contributor

@sebiwtt The following work on this PR would be to create certain examples and do some benchmark with different settings. Would you be interested in continuing the work?

Copy link
Contributor

@hoanganhngo610 hoanganhngo610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hoanganhngo610 hoanganhngo610 merged commit a36c5dd into online-ml:main Mar 18, 2024
4 checks passed
@hoanganhngo610
Copy link
Contributor

All checks have passed, I will proceed to merge it at its current state. Once again, thank you @sebiwtt and @MaxHalford for all your input.

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Mar 18, 2024

@hoanganhngo610 indeed I would be interested! If you are interested I could send you the thesis I wrote on the topic (I already conducted some benchmarks)😀

@hoanganhngo610
Copy link
Contributor

@sebiwtt That would be perfect, thank you very much! I will be following up with you by email tomorrow (as it's already a bit late here in Vietnam right now)!

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Mar 18, 2024

@hoanganhngo610 do you need an email address of mine?

@hoanganhngo610
Copy link
Contributor

@sebiwtt I believe it says on your profile that your personal email is [email protected], is that correct? Can I write you through that email?

@sebiwtt
Copy link
Contributor Author

sebiwtt commented Mar 18, 2024

@hoanganhngo610 perfect! I forgot about that😅 You can contact me via that address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants