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

Follow multi-inheritance pattern for all applicable detectors #61

Closed
Anmol-Srivastava opened this issue Jul 7, 2022 · 5 comments
Closed
Assignees
Labels
enhancement New feature or request
Projects

Comments

@Anmol-Srivastava
Copy link
Contributor

Task

After #46, all remaining detectors outside of KdqTree will need to be updated, to now use the StreamingDetector and BatchDetector ABCs if they are meant to service both options. Such detectors will (likely) also need to implement their own parent algorithm class, e.g. KdqTreeDetector as part of the multi-inheritance pattern.

Impact

This will significantly progress the broad refactor of the drift detectors' object design. See also #15

@Anmol-Srivastava Anmol-Srivastava created this issue from a note in Ver 0.2.0 (To do) Jul 7, 2022
@Anmol-Srivastava Anmol-Srivastava self-assigned this Jul 7, 2022
@Anmol-Srivastava Anmol-Srivastava added enhancement New feature or request blocked Needs something clarified or resolved to proceed labels Jul 7, 2022
@Anmol-Srivastava Anmol-Srivastava changed the title Follow multi-inheritance pattern for all remaining detectors Follow multi-inheritance pattern for all applicable detectors Jul 7, 2022
@Anmol-Srivastava Anmol-Srivastava removed the blocked Needs something clarified or resolved to proceed label Jul 10, 2022
@Anmol-Srivastava
Copy link
Contributor Author

Anmol-Srivastava commented Jul 10, 2022

To complete the refactor, it is first necessary to determine:

  • which detectors need to be refactored into streaming detector child classes
  • which need to be made into batch detector child classes
  • which need both

Note that a detector being exclusively batch/streaming should not prevent us from isolating as much 'other' logic into the parent algorithm class as possible. Currently:

Streaming only:

  • - CUSUM
  • - PageHinkely
  • - ADWIN
  • - DDM
  • - EDDM
  • - LFR
  • - STEPD
  • - PCACD

Batch only:

  • - CDBD
  • - H3DM
  • - HistogramDensityMethod

Both:

  • kdqTree
  • ???

@Anmol-Srivastava
Copy link
Contributor Author

Optionally, multiple branches can be made to develop on each of the involved detectors and linked into one PR for this issue.

@tms-bananaquit
Copy link
Collaborator

Checking these off as I go -- not necessarily setting them up with maximum possible refactoring into a parent class, but making sure that they each inherit the proper ABC.

@tms-bananaquit tms-bananaquit mentioned this issue Aug 9, 2022
11 tasks
@Anmol-Srivastava
Copy link
Contributor Author

@tms-bananaquit sounds good; later we can push the parent class concept if we know that certain of these detectors may need to be both streaming/batch

@tms-bananaquit
Copy link
Collaborator

@Anmol-Srivastava Going to go ahead and close unless there's objection. #70 can finish deprecating DriftDetector entirely.

Ver 0.2.0 automation moved this from To do to Done Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

No branches or pull requests

2 participants