-
Notifications
You must be signed in to change notification settings - Fork 2
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
Detector components (costs, scores etc.) as classes #24
base: main
Are you sure you want to change the base?
Conversation
Common pattern for initialisers.
…ction to scores.mean
Leave it to the numba compiler for now. Checking it in a good way is complicated due to the generic classes such as CostBasedChangeScore
Decoupled from numba and the detectors as opposed to other suggested designs.
Error in previous commit.
Unnecessary
Much better coverage of all costs used as scores in the detectors.
None means that the min_size is unknown, for example until it is fitted.
Necessary for setting good default penalties and thresholds
Tag won't pass sktime conformance test
For question 1. I am partial to the And regarding the calling convention for the
So if possible, I'm wondering if a more explicit calling convention where each column that defines the interval would be a named argument to the |
Thanks for the pointer. As I am not familiar with the code, and the names do not seem to imply examples, may I request two examples (optimally directly here in the text) for each base API? E.g., what would be an "interval evaluator"? Is this a performance metric, or sth else? I suppose it is sth else? |
Here's the CUSUM change score for example:
|
I see, thanks! Major comment: could the top base class be the same as for evaluation metrics? I understand now that the objects here are used as components in the various change point algorithms, however, would an evaluation metric like windowed F1 also be following the interface? I would tend to "no", i.e., that should be a distinct class, but I just wanted to share this surface thought. Minor comment: |
More points:
|
Let's think about it in the future! For now, I think they should be distinct.
Ok, thanks!
I might go with this one. I don't like the very generic "Evaluator", so "Scorer" might be better. My hesitation is due to "scoring" being used a lot of places, and that the way I've used it in the code so far is to mean "higher is better", while costs are "lower is better".
They should be separate classes. All detectors can take cost inputs, but only a few can take in savings. The typing and input handling is much simpler with different classes, and they really serve different purposes. |
Regarding the calling conventions for These base classes inherit from
Then the meaning of the Current version:
Potential version:
|
Goal: Unify the detector components. Make them safer. Make the extension pattern simpler and clearer.
See #23 for discussions.
New features:
The
BaseIntervalEvaluator
class, inheriting from sktime.BaseEstimator. Public methods:fit(self, X, y=None) -> self
evaluate(self, intervals: ArrayLike) -> np.ndarray
Four sub base classes inheriting from BaseIntervalEvaluator:
skchange.costs.BaseCost
. Expects 2 columns in intervals: start, end.skchange.change_scores.BaseChangeScore
. Expects 3 columns in intervals: start, split, end.skchange.anomaly_scores.BaseSaving
. Expects 2 columns in intervals: start, end.skchange.anomaly_scores.BaseLocalAnomalyScore
: Expects 4 columns in intervals: outer_start, inner_start, inner_end, outer_end.Classes for automatically converting costs to any of the three other score classes.
Convenience functions allowing either costs or an appropriate score to be used as input to all the detectors.
All existing functionality is implemented within the new design + additions.