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

Fix ECOD and COPOD implementation according to #453 #493

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

Lucew
Copy link
Contributor

@Lucew Lucew commented Mar 30, 2023

Hi, it's me again ;-)

I found time to look into the basic intention behind issue #453 again. In order to fix this, I made some adaptions to ECOD and COPOD that fix the issue as initially stated. This is the corresponding pull request.

I adapted the implementation of both algorithms to be more in line with the original papers proposed as we discussed in the issue (see details in my earlier post). You can also find the ECOD-Paper here and the COPOD-Paper here.

In addition to that, I also did a minor cleanup to get rid of some code repetition in case of parallel execution. This corresponds to these lines which are repeated in these lines by simply merging _decision_function_parallel into decision_function in ECOD. The same goes for COPOD as well.

I already saw a small decrease in line coverage by the tests in my local repository, but as far as I can tell this is mainly due to reducing the number of lines overall (and moving some lines). The lines that change the algorithm itself are covered by tests. Get back to me, if I should make sure the few lines are covered as well (have not been before) and I will update this pull request.

Thanks for providing this package and your effort to always look into the pull requests!

Lucas

All Submissions Basics:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you checked all Issues to tie the PR to a specific one?

All Submissions Cores:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you successfully ran tests with your changes locally?
  • Does your submission pass tests, including CircleCI, Travis CI, and AppVeyor?
  • Does your submission have appropriate code coverage? The cutoff threshold is 95% by Coversall.

@coveralls
Copy link

coveralls commented Mar 30, 2023

Pull Request Test Coverage Report for Build 4566127243

  • 48 of 50 (96.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 93.541%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyod/models/copod.py 25 26 96.15%
pyod/models/ecod.py 23 24 95.83%
Totals Coverage Status
Change from base Build 4463703550: -0.01%
Covered Lines: 8690
Relevant Lines: 9290

💛 - Coveralls

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.

None yet

3 participants