-
Notifications
You must be signed in to change notification settings - Fork 4
Semiparametric Mu Estimation for NMV mixtures #5
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
Semiparametric Mu Estimation for NMV mixtures #5
Conversation
andreev-sergej
commented
Jul 20, 2024
- Реализован алгоритм для оценки параметра
$\mu$ в семипараметрическом случае для NMV смесей. - Упразднено взаимодействие пользователя с классом Registry. Для этого изменены методы init в классах смесей.
- Написаны тесты для методов нового класса SemiParametricMuEstimation.
Не прошел 1 тест, где настоящий параметр |
self.sample = sample | ||
self.m, self.tolerance, self.omega = self._set_default_params(params) | ||
|
||
def _set_default_params(self, params: list[Any] | None) -> tuple: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может параметры передавать не списком, а словарём, или специальным объектом, чтобы обращаться по именам. Сейчас проверка довольно сложная + добавление ещё одного параметра ещё больше её перегрузит.
А использование словаря или, например, датакласса позволит гораздо удобнее работать со значениями по умолчанию
|
||
if params is None: | ||
return default_m, default_tolerance, default_omega | ||
elif len(params) == 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По-моему, тут elif
избыточен
default_m = 1000 | ||
default_tolerance = 1 / 10**5 | ||
default_omega = self._default_omega |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может значения по умолчанию в подобие констант вынести? Сейчас выглядят как волшебные значения
…. New parameter: max_iterations
if "m" in kwargs and (not isinstance(kwargs.get("m"), int) or kwargs.get("m", -1) <= 0): | ||
raise ValueError("Expected positive integer as parameter m") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if "m" in kwargs and (not isinstance(kwargs.get("m"), int) or kwargs.get("m", -1) <= 0): | |
raise ValueError("Expected positive integer as parameter m") | |
m = kwargs.get("m", M_DEFAULT_VALUE) | |
if not isinstance(m, int) or m <= 0: | |
raise ValueError("Expected positive integer as parameter m") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне кажется, так попроще
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А ещё можно ключи, по которым получаются параметры, тоже в константы вынести