Skip to content

Conversation

vkutuev
Copy link
Collaborator

@vkutuev vkutuev commented Jul 10, 2025

Based on #43

Copy link
Collaborator

@alexdtat alexdtat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look closely at the algorithms, because there're no tests to run it. Imo metrics here look redundand.

:param detected_change_points: list of detected change point indices.
:return: a dictionary with evaluation metrics (precision, recall, F1 score).
def evaluate_detection_accuracy(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this here? It seems this logic belongs to benchmarking.

@@ -1,112 +1,155 @@
from abc import abstractmethod
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add license/copyright metadata here (and header docstring)

:param window: input data window
:return: True if window can be processed, else False
"""
return len(window) >= 2 * self.min_window_size
Copy link
Collaborator

@alexdtat alexdtat Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why len must be at least twice as the minimal? It looks a little bit confusing.

def loss(alpha_array: npt.NDArray[np.float64], /) -> float:
alpha = alpha_array[0]
ratio = np.exp(np.log(test_density) - np.log(ref_density + 1e-10) - alpha)
return float(-np.mean(np.log(ratio + 1e-10)) + self.regularization * alpha**2)
Copy link
Collaborator

@alexdtat alexdtat Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these 1e-10? Is it for numerical stability? Should it be a parameter? Or maybe make it a private field of class?

)

return cast(list[int], np.where(weights > self.threshold)[0].tolist())
def _kde_on_grid(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method '_kde_on_grid' may be 'static'


def objective_function_wrapper(alpha: npt.NDArray[np.float64], /) -> float:
"""Wrapper for the objective function to calculate the density ratio.
def _validate_window(self, window: npt.NDArray[Any]) -> npt.NDArray[np.float64]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method '_validate_window' may be 'static'

test_density = self._kernel_density_estimation(test_value, bandwidth)
reference_density = self._kernel_density_estimation(reference_value, bandwidth)
window = self._validate_window(window)
if not self._is_window_valid(window):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why previously validated window can be not valid here? Naming looks a little bit confusing.

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.

3 participants