Skip to content

Conversation

bt2901
Copy link
Contributor

@bt2901 bt2901 commented Aug 4, 2020

No description provided.

@bt2901 bt2901 requested a review from Alvant as a code owner August 4, 2020 22:13
save_experiment: bool = False,
experiment_directory: str = DEFAULT_EXPERIMENT_DIR):
experiment_directory: str = DEFAULT_EXPERIMENT_DIR,
nums_topics_list: int = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

О, круто, я тоже хотел это добавить!
Только вот лучше, наверно, так

nums_topics: List[int] = None

Copy link
Collaborator

@Alvant Alvant Aug 20, 2020

Choose a reason for hiding this comment

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

Плюс стоит создать докстринг и отметить там, что это параметр приоритетнее min_num_topics, max_num_topics, num_topics_interval.

P.S. А вообще это всё, конечно, немного тяп-ляп) По-хорошему, OptimizeScoresMethod должен принимать на вход заданный неким образом интервал тем. А интервал тем может определяться как границами и шагом, так и вручную заданным списком чисел тем (так и мб ещё каким способом; например "центрами" — предполагаемые оптимумы, — вокруг которых надо перебирать числа тем, и параметрами перебора тем около этих центров)

def __init__(..., search_interval, ...)

search_interval = SearchInterval.from_borders_and_step(min_num_topics, max_num_topics, num_topics_interval)
...
search_interval = SearchInterval.from_list(nums_topics)
...
search_interval  = SearchInterval.from_whatever_else(...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Или надо вообще убрать min_num_topics, max_num_topics, num_topics_interval, и оставить только list 😅

topnum/utils.py Outdated
'[email protected]_contrast': max,
'[email protected]_purity': max,
'[email protected]_size': None,
'perp': None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

min?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

дело в том, что перплексии дублируются (лишие скоры при инициализации модели). Чтобы не рисовать загромождающих лишних графиков, я ставлю None (это вместо USELESS_KEYS, которые были раньше)

Тяп-ляп, да.

Copy link
Collaborator

Choose a reason for hiding this comment

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

А, ок. Может, perp как раз оставим?) Типа есть holdout_perp, и будет perp?

'[email protected]_purity': max,
'[email protected]_size': None,
'perp': None,
'sparsity_phi': None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

max?

topnum/utils.py Outdated


SCORES_DIRECTION = {
'PerplexityScore@all': min,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Прикольно!

P.S. Я бы создал enum с возможными значениями direction:

class ScoreDirection(Enum):
    NONE = auto()
    MIN = auto()
    MAX = auto()

SCORES_DIRECTION = {
'PerplexityScore@all': min,
'SparsityThetaScore': max,
'SparsityPhiScore@word': max,
Copy link
Collaborator

@Alvant Alvant Aug 20, 2020

Choose a reason for hiding this comment

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

То, что здесь зашиты конкретные модальности — не очень хорошо... (новый датасет, новая модальность — fail). Но с этим можно жить)
...
Да и имена скоров заданы)) Короче, это очень currently-conducted-experiments-related штука

Copy link
Contributor Author

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.

У обычной-то модели классы точно есть, а у Dummy... скорее всего, нет

topnum/utils.py Outdated
'toptok1': max
}

def classify_curve(my_data, FRAC_THRESHOLD, score_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Капсом имя параметра

topnum/utils.py Outdated
colored_values[colored_values > threshold] = np.nan

intervals = colored_values[colored_values.notna()]
minx, maxx = min(intervals.index), max(intervals.index)
Copy link
Collaborator

@Alvant Alvant Aug 20, 2020

Choose a reason for hiding this comment

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

minx, maxx?

topnum/utils.py Outdated
'toptok1': max
}

def classify_curve(my_data, FRAC_THRESHOLD, score_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

А можешь плиз тип my_data указать? 🙂 (кстати, имя лучше другое дать)
А то я понял, что не знаю, что такое intervals.index

topnum/utils.py Outdated
# and abs(intervals.loc[maxx] - optimum_val) <= :
curve_type = "outside"
else:
curve_type = "jumping"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ну, это тоже явно на константы или enum-ы напрашивается

Copy link
Contributor Author

@bt2901 bt2901 Aug 20, 2020

Choose a reason for hiding this comment

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

+1


intervals = colored_values[colored_values.notna()]
minx, maxx = min(intervals.index), max(intervals.index)
optimum_idx = set(intervals.index)
Copy link
Collaborator

Choose a reason for hiding this comment

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

optimum_idx: не совсем удачное название? или что здесь хочется посчитать?)
minx, maxx = min(intervals.index), max(intervals.index) -> проверка if minx == maxx равносильна проверке на 349 строчке?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Нет, не равносильна.

Пусть мы перебираем темы с шагом 2. intervals.index == [10, 12, 14, 22] (два максимума), slice_idx = [10, 12, ..., 20, 22]

Тут скорее можно len() сравнивать, но мне показалось что это менее интуитивно.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ок, с примером стало понятнее)

topnum/utils.py Outdated
if (optimum_idx == slice_idx):
curve_type = f"interval {len(intervals)}"
if len(intervals) == 1:
curve_type = "sharp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

А на outside curve_type уже не сможет измениться по коду ниже?..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, но наверное это зря, исправлю

topnum/utils.py Outdated

minx, maxx = min(colored_values.index), max(colored_values.index)
if minx in optimum_idx:
curve_type = "outside"
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему если minx in optimum_idx, то curve_type = outside?) Score direction не влияет на то, minx или maxx надо проверять?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Нет (представь себе U-образную кривую)
minx, maxx -- это границы интервала по x. Score direction влияет на то, это интервал с минимумом графика или максимумом

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.

...Мб это плато?)

topnum/utils.py Outdated
*name_base, param_id, seed = experiment_name.split("_")

seed = int(seed) if seed != 'None' else 0
if seed > 3:
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

topnum/utils.py Outdated
my_ax.plot(data.T.mean(axis=0), linestyle=style, label=experiment_name)
*name_base, param_id, seed = experiment_name.split("_")

seed = int(seed) if seed != 'None' else 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

None ведь не должно быть нигде? Это ведь от старого кода, когда seed вообще не выставлялся?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Да, это поддержка обратной совместимости с legacy
seed > 3 это тоже оттуда осталось, когда сиды через RandomState выставлялись большие

Copy link
Collaborator

Choose a reason for hiding this comment

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

Может, в топку legacy?) Зачем поддерживать? Поддержка старого -> есть вероятность случайно какой-то баг словить?


my_data = data.T.mean(axis=0)

if maxval is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Когда может быть нужен maxval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Когда например я рисую перплексию и там из-за того, что первое значение 100500 масштаб графика ломается

для симметрии ещё нужен minval, видимо

и докстринг видимо :)

topnum/utils.py Outdated

if FRAC_THRESHOLD:
colored_values, curve_type = classify_curve(my_data, FRAC_THRESHOLD, score)
if curve_type == "jumping":
Copy link
Collaborator

@Alvant Alvant Aug 20, 2020

Choose a reason for hiding this comment

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

P.S.
Модификация classify_curve приведёт к необходимости менять код и здесь (добавлять/удалять/изменять if ветки)

topnum/utils.py Outdated
if curve_type == "outside":
marker = "^"
my_ax.plot(colored_values, linestyle=style, color=color, alpha=1.0)
my_ax.plot(colored_values, marker=marker, linestyle='', color='black', alpha=1.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему два раза плот? 😅 linestyle=''? это типа... нет линии?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Аааа, это чтоб точки нарисовать? Что так сложно?)) Почему нельзя за один plot всё сделать? Может, если за один нельзя, вместо второго plot использовать scatter?

@Alvant
Copy link
Collaborator

Alvant commented Aug 20, 2020

@bt2901 Виктор, единственное, хотелось бы, чтобы была функция, которая просто принимает на вход значения X, Y, и указатель, что мы ищем (max или min). Мало ли просто захочется просто какой-то график обработать (без привязки к my_data, score_names_stuff, и т.д.). Такое можно сделать? Вот, например, я захочу с помощью твоей тулзы проанализировать стабилити и ренормализацию.

P.S. В идеале на вход лучше подавать просто X и Y. А тулза (в идеале) должна сказать, где там максимум, где там минимум, где там плато, с "какой уверенностью" тулза считает, что это именно макс/мин/плато

Ещё у меня вопрос: есть иногда графики, где первая точка (как правило) "лишняя" (грубо говоря, скор посчитан для модели с одной темой, и там какая-то жесть; а дальше начинается "нормальная" зависимость). Можно ли как-то такие явные граничные выбросы обрубать, выбрасывать из анализа? Хотя, наверно, надо это делать самому перед тем, как подать range значений на вход анализатору...

@Alvant Alvant linked an issue Aug 20, 2020 that may be closed by this pull request
@@ -0,0 +1,2164 @@
{
Copy link
Collaborator

@Alvant Alvant Sep 4, 2020

Choose a reason for hiding this comment

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

@bt2901 Виктор, тут может быть конфликт: этот файлик есть в двух реквестах. Здесь, как я понимаю, более старая версия ноутбука и его можно убрать вообще из реквеста?

@Alvant Alvant merged commit 590242e into master Sep 8, 2020
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.

Search interval v2: simple list as input
2 participants