Feature/pattern change detection #17
Conversation
…, STPH) with real-time GUI dashboard
…, STPH) with real-time GUI dashboard
There was a problem hiding this comment.
There are several points that need to be fixed before merging.
Several things mentioned under https://github.com/tuda-parallel/FTIO/blob/feature/pattern_change_detection/docs/students_contribute.md are missing. This includes:
- Documentation and license for new files: https://github.com/tuda-parallel/FTIO/blob/feature/pattern_change_detection/docs/students_contribute.md#-module-documentation-and-licensing
- Adding test cases: https://github.com/tuda-parallel/FTIO/blob/feature/pattern_change_detection/docs/students_contribute.md#-module-documentation-and-licensing
- Adding a documentation: https://github.com/tuda-parallel/FTIO/blob/feature/pattern_change_detection/docs/students_contribute.md#-module-documentation-and-licensing
There was a problem hiding this comment.
The changes here are not needed. Please remove them
| periodicity_score = new_periodicity_scores(amp, b_sampled, prediction, args) | ||
|
|
||
| t_sampled = time_stamps[0] + np.arange(0, n) * 1 / args.freq | ||
| # Safety check for empty time_stamps |
There was a problem hiding this comment.
This is not needed. please remvove it.
|
|
||
| #! Save up to n_freq from the top candidates | ||
| if args.n_freq > 0: | ||
| if args.n_freq > 0 and n > 0: |
There was a problem hiding this comment.
This is not needed. Please remove it.
| prediction.phi = phi[dominant_index] | ||
| prediction.t_start = time_stamps[0] | ||
| prediction.t_end = time_stamps[-1] | ||
| if n > 0 and len(dominant_index) > 0: |
There was a problem hiding this comment.
The changes below are not needed. Please remove them. Especially, as time does not always start at 0.
| @@ -0,0 +1,1064 @@ | |||
| """Change point detection algorithms for FTIO online predictor.""" | |||
There was a problem hiding this comment.
here you can add that you are the author of this file. See lines 1-17 in https://github.com/tuda-parallel/FTIO/blob/main/ftio/cli/ftio_core.py.
Also please add typehints and definition of the arguments (see https://github.com/tuda-parallel/FTIO/blob/main/ftio/cli/ftio_core.py#L45)
| Returns: | ||
| out (dict): probability of predictions in ranges | ||
| """ | ||
| def find_probability(data: list[dict], method: str = "db", counter:int = -1) -> list: |
There was a problem hiding this comment.
Why delete the function doc string? Please only modify your functions
There was a problem hiding this comment.
This function is a mess. There is no need to define the same variables three times with different names (e.g., self.pagehinkley_frequencies, self.adwin_frequencies, and self.detector_frequencies). In general, you added too many parameters, making this more complicated than it needed to be. Please:
- Undo the deleted comments in the original version?
- Reduce the parameters you added (for instance, to one that loads from another file the values you need). However, avoid redundancy.
- Keep in mind that these parameters are shared among the processes. More means slower runs
There was a problem hiding this comment.
Why are these added here? I would expect them in https://github.com/tuda-parallel/FTIO/blob/main/pyproject.toml. You could add [dependencies-gui] under [project.optional-dependencies]. Also how do you call the gui? I would have expected something under [project.scripts] in the same file for the gui
Implementation of the three adaptive change point detection algorithms with a real-time visualization dashboard.
Algorithms: ADWIN, AV-CUSUM, STPH