ShadPython - 2 - Alexander Andrianov - FIVT

[shad.python, 2015-05-31 12:57:03]

https://codereview.appspot.com/237480043/diff/1/textgen.py
File textgen.py (right):

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode24
textgen.py:24: k = " ".join([t for t in tokens[i:i + depth]])
k, v - непонятные названия, нужно придумать что-то, что описывает содержимое
переменных

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode43
textgen.py:43: chains = [make_chains(depth, text, True) for depth in xrange(1,
max_depth+1)]
Не очень хорошо, что функция обучения модели (сбора вероятностей) находится
прямо в функции генерации. Для того, чтобы сгенерировать несколько текстов на
основе одной модели, придется каждый раз заново обучать ее. Лучше разделить эти
действия.

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode58
textgen.py:58: sentence.append(random.choice(chains[key_length-1][key]))
Получается, что у вас если токен 100 раз встретился после этой истории, то он
будет 100 раз храниться в модели. Это не очень экономно по отношению к памяти, к
тому же модель становится менее понятной. Лучше хранить слова и их частоты.

[lxndr.ndrnv, 2015-05-31 16:12:32]

Вопрос по последнему комментарию

https://codereview.appspot.com/237480043/diff/1/textgen.py
File textgen.py (right):

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode24
textgen.py:24: k = " ".join([t for t in tokens[i:i + depth]])
On 2015/05/31 12:57:03, shad.python wrote:
> k, v - непонятные названия, нужно придумать что-то, что описывает содержимое
> переменных

Done.

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode43
textgen.py:43: chains = [make_chains(depth, text, True) for depth in xrange(1,
max_depth+1)]
On 2015/05/31 12:57:03, shad.python wrote:
> Не очень хорошо, что функция обучения модели (сбора вероятностей) находится
> прямо в функции генерации. Для того, чтобы сгенерировать несколько текстов на
> основе одной модели, придется каждый раз заново обучать ее. Лучше разделить
эти
> действия.

Done.

https://codereview.appspot.com/237480043/diff/1/textgen.py#newcode58
textgen.py:58: sentence.append(random.choice(chains[key_length-1][key]))
On 2015/05/31 12:57:03, shad.python wrote:
> Получается, что у вас если токен 100 раз встретился после этой истории, то он
> будет 100 раз храниться в модели. Это не очень экономно по отношению к памяти,
к
> тому же модель становится менее понятной. Лучше хранить слова и их частоты.

Т.е. изначально вместо записей {начало цепи: [всевозможные окончания]}хранить
{начало: [(окончание, вероятность)]} ?

[shad.python, 2015-06-05 13:01:03]

https://codereview.appspot.com/237480043/diff/20001/textgen.py
File textgen.py (right):

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode33
textgen.py:33: def get_probabilities(chains, f):
Слово get здесь немного сбивает с толку, как будто эта функция должна вернуть
вероятность. Лучше использовать write или output.

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode41
textgen.py:41: def generate_text(max_depth, cdfs, text_size):
cdfs - непонятное название

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode125
textgen.py:125: frequencies = sorted(list(frequencies), key=lambda x: x[1])
Этот код частично дублируется. Сделайте отдельную функцию "превратить частоты в
вероятности". Кроме того, все остальное тоже лучше выделить в отдельную функцию,
чтобы при необходимости обучать модель в другом месте кода можно было бы просто
вызвать эту функцию. Еще было бы удобно собрать функции обучения и генерации в
класс, чтобы скрыть от пользователя подробности реализации.

[lxndr.ndrnv, 2015-06-05 14:24:18]

https://codereview.appspot.com/237480043/diff/20001/textgen.py
File textgen.py (right):

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode33
textgen.py:33: def get_probabilities(chains, f):
On 2015/06/05 13:01:03, shad.python wrote:
> Слово get здесь немного сбивает с толку, как будто эта функция должна вернуть
> вероятность. Лучше использовать write или output.

Done.

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode41
textgen.py:41: def generate_text(max_depth, cdfs, text_size):
On 2015/06/05 13:01:02, shad.python wrote:
> cdfs - непонятное название

Done.

https://codereview.appspot.com/237480043/diff/20001/textgen.py#newcode125
textgen.py:125: frequencies = sorted(list(frequencies), key=lambda x: x[1])
On 2015/06/05 13:01:03, shad.python wrote:
> Этот код частично дублируется. Сделайте отдельную функцию "превратить частоты
в
> вероятности". Кроме того, все остальное тоже лучше выделить в отдельную
функцию,
> чтобы при необходимости обучать модель в другом месте кода можно было бы
просто
> вызвать эту функцию. Еще было бы удобно собрать функции обучения и генерации в
> класс, чтобы скрыть от пользователя подробности реализации.

Done.