Skip to content

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

Merged

Conversation

andreev-sergej
Copy link
Collaborator

  1. Реализован алгоритм для оценки параметра $\mu$ в семипараметрическом случае для NMV смесей.
  2. Упразднено взаимодействие пользователя с классом Registry. Для этого изменены методы init в классах смесей.
  3. Написаны тесты для методов нового класса SemiParametricMuEstimation.

@andreev-sergej
Copy link
Collaborator Author

Не прошел 1 тест, где настоящий параметр $\mu$ оказывается больше чем область поиска $m$, и ожидается, что лучшей оценкой для $\mu$ будет $m$.
Однако, когда $\mu$ и $m$ близки, то может не повезти и наилучший параметр $\mu$ для реализованной выборки будет по модулю меньше, чем $m$. Тогда оценка $\mu$ не равна $m$ и тест проваливается.
Решением будет увеличение объема выборки, либо проверка сильно различающихся $\mu$ и $m$

self.sample = sample
self.m, self.tolerance, self.omega = self._set_default_params(params)

def _set_default_params(self, params: list[Any] | None) -> tuple:
Copy link
Collaborator

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

По-моему, тут elif избыточен

Comment on lines 38 to 40
default_m = 1000
default_tolerance = 1 / 10**5
default_omega = self._default_omega
Copy link
Collaborator

Choose a reason for hiding this comment

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

Может значения по умолчанию в подобие констант вынести? Сейчас выглядят как волшебные значения

@andreev-sergej andreev-sergej requested a review from vkutuev July 21, 2024 03:37
Comment on lines 52 to 53
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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Мне кажется, так попроще

Copy link
Collaborator

Choose a reason for hiding this comment

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

А ещё можно ключи, по которым получаются параметры, тоже в константы вынести

@andreev-sergej andreev-sergej merged commit a865cac into PySATL:main Jul 23, 2024
1 check passed
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