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 3rdparty detector bugs #45

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pmenn36
Copy link

@pmenn36 pmenn36 commented Oct 2, 2020

I noticed when evaluating my own detector using this process that there were two bugs that got in the way:

  1. The default NAB labels contained duplicate rows, but the output of my detector did not contain duplicates. This resulted in a length difference between the labels and the anomaly scores, which raised an AssertionError. I fixed this by adding an optional --removeDuplicateLabels flag. The default behavior remains unchanged.
  2. The anomaly scores from my output csv were being read in as strings instead of floats. I forced casting to float in sweeper.py, which does not affect the default behavior because the anomaly scores from the default detectors are correctly read in as floats anyway.

Patrick Menninger added 2 commits October 1, 2020 20:15
1. NAB benchmark contained duplicate timestamps which caused length to not match up with imported results, throwing errors
2. Threshold values were read in as strings instead of floats, throwing errors

Signed-off-by: Patrick Menninger <[email protected]>
Duplicate fixes from previous commit broke the default detectors, so I made them optional

Signed-off-by: Patrick Menninger <[email protected]>
@ctrl-z-9000-times
Copy link
Collaborator

Hi,

Sorry it took a year for anyone to respond to your PR.
For what its worth, I just now merged your changes for casting the thresholds to floating point numbers.

I didn't merge the stuff about duplicate records BC I've never encountered that bug.
If that is a real bug and something that you still want fixed then ping me (@ctrl-z-9000-times )
and I will review and merge that as well.

Thanks!

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.

2 participants