ShadPython - 2 - Roman Nadvodnuk - Minsk

[shad.python, 2015-05-05 09:13:54]

https://codereview.appspot.com/225420043/diff/1/markov_gen.py
File markov_gen.py (right):

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode1
markov_gen.py:1: # coding: utf-8
Здесь не тот же самый код, который вы отправили в контест. Просьба присылать тот
же код (если делаете какие-то исправления, то их нужно отправить в контест для
досдачи). Пока напишу только несколько общих замечаний.

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode12
markov_gen.py:12: sys.stdin = open('input.txt', 'r+')
При работе с файлами лучше пользоваться конструкцией with..as.., чтобы точно
контролировать момент их закрытия.

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode20
markov_gen.py:20: def parse_for_contest(self, str):
Вообще подразумевалось, что функции токенизации и подсчета частот, которые
проверяет контест, далее будут использоваться в генерации.

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode144
markov_gen.py:144: def tokenize(self):
Это не токенизация, а подсчет частот.

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode145
markov_gen.py:145: str = sys.stdin.read().replace('\n', ' ')
Ввод/вывод и содержательные действия лучше отделять друг от друга. Это позволит
в дальнейшем этот код переиспользовать, даже если формат ввода и вывода
поменяется.

https://codereview.appspot.com/225420043/diff/1/markov_gen.py#newcode150
markov_gen.py:150: russian_capitals = 'ЙЦУКЕНГШЩЗХЪФЫВАПРОЛДЖЭЯЧСМИТЬБЮ'
Лучше просто работать с юникодными строками и пользоваться методом
unicode.isupper().

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

[romik.cookie, 2015-05-10 22:12:13]

Перезалил код и отправил в систему контеста для подтверждения корректности.

[shad.python, 2015-05-15 20:08:13]

https://codereview.appspot.com/225420043/diff/20001/markov_gen.py
File markov_gen.py (right):

https://codereview.appspot.com/225420043/diff/20001/markov_gen.py#newcode79
markov_gen.py:79: self.d = d
d - непонятно названия, нужно сделать его более подробным.

https://codereview.appspot.com/225420043/diff/20001/markov_gen.py#newcode138
markov_gen.py:138: str = sys.stdin.read()
Я имел ввиду, что эта функция получает данные для работы непосредственно с
консоли. Из-за этого ее нельзя будет переиспользовать в задачах, где нужно
читать данные из файла или предварительно их менять и т.п.

https://codereview.appspot.com/225420043/diff/20001/markov_gen.py#newcode144
markov_gen.py:144: russian_capitals = 'ЙЦУКЕНГШЩЗХЪФЫВАПРОЛДЖЭЯЧСМИТЬБЮ'
По этому замечанию ничего не поменялось.

[romik.cookie, 2015-05-21 18:24:48]

Попытка #3, пофиксил замечания.

[shad.python, 2015-05-25 15:49:19]

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py
File markov_gen.py (right):

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode13
markov_gen.py:13: token = ''
Здесь все еще используются неюникодные строки

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode82
markov_gen.py:82: def __init__(self, d=0):
Это d - тоже:)

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode87
markov_gen.py:87: for i in xrange(1, self.depth + 1):
Здесь дублируется код (см. chains_for_contest). Лучше сделать ту функцию более
гибкой и здесь ею воспользоваться.

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode94
markov_gen.py:94: """
Это основной код, он не должен быть закомментирован (а также с. 169-174 и с.
176-181).
Обратите внимание, что по условии задания пользоваться можно только стандартной
библиотекой.

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode124
markov_gen.py:124: str = input_stream.read()
Проблема не в названии, а в том, как эта функция получает свои аргументы. Сейчас
она может их получить только из конкретного файла, а это значит, что класс вне
данной задачи будет очень неудобно использовать. Нужно сделать, чтобы все
необходимые данные передавались через аргументы.

https://codereview.appspot.com/225420043/diff/40001/markov_gen.py#newcode187
markov_gen.py:187: with open('input.txt', 'r+') as input_stream:
Переменная input_stream используется как глобальная, сделайте ее лучше
аргументом for_contest()

[shad.python, 2015-05-30 17:34:14]

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py
File markov_gen.py (right):

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode10
markov_gen.py:10: punctuation = "!\"#$%&()*+ ,-./:;<=>?@[\]^_`{|}~"
Эта строка не юникодная.

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode71
markov_gen.py:71: line = raw_input().decode('cp1251')
Здесь используется неверная кодировка (в контесте кодировка UTF-8).

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode95
markov_gen.py:95: chains[1][key] = collections.Counter(value)
Здесь код отличается от того, что в контесте (большая просьба присылать один и
тот же код!). В контесте здесь get_chains вызывается столько раз, чему равна
глубина, хотя она и так считает статистику для всех глубин - отсюда и
получается, что все блоки "удваиваются".

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode130
markov_gen.py:130: for i in xrange(count):
Такой способ генерации - не очень хорошая идея, потому что его сложность быстро
растет при росте обучающей выборки, даже если в ней не появляются новые слова.
Более эффективно было бы хранить вероятности (сделав для ее подсчета здесь и для
режима вероятностей общую функцию). Для этого придется здесь написать свой
аналог numpy.random.choice.

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode153
markov_gen.py:153: input_string = []
Основная проблема со всем старым и текущим кодом - ограниченность использования
функции (только чтением из файла или из консоли). Нужно сделать, чтобы функция
получала ровно то, что ей нужно для работы (ей нужен текст, а не файл с
текстом), тогда с ней будет удобно работать вне данной задачи.

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode202
markov_gen.py:202: params = raw_input().split(' ')
Чтобы не парсить параметры вручную лучше воспользоваться библиотекой argparse

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode220
markov_gen.py:220: # åñëè åñòü äîïîëíèòåëüíûé ïàðàìåòð, îáîçíà÷àþùèé èìÿ ôàéëà
äëÿ ÷òåíèÿ
В какой кодировке эта строка? Она должна корректно отображаться, если это utf-8

https://codereview.appspot.com/225420043/diff/60001/markov_gen.py#newcode234
markov_gen.py:234: g.count_probs()
Результаты работы этого вызова нигде не используются.

[shad.python, 2015-05-31 14:52:58]

https://codereview.appspot.com/225420043/diff/100001/markov_gen.py
File markov_gen.py (right):

https://codereview.appspot.com/225420043/diff/100001/markov_gen.py#newcode12
markov_gen.py:12: input_string = []
Здесь, кстати, можно считывать так же, как и для файлов (при подключенной
библиотеке sys):

sys.stdin.read()

https://codereview.appspot.com/225420043/diff/100001/markov_gen.py#newcode30
markov_gen.py:30: input_string = input_string.decode('utf8')
Вообще я не советую делать такие проверки - если пользователь функции должен
передать один тип, а он передал другой - скорее всего он ошибся, и надо просто
об этом сообщить, не пытаясь угадать, что он имел ввиду. Конечно, второе иногда
удобнее, но есть шанс пропустить ошибку и ее тогда потом будет совсем тяжело
найти. Я бы здесь написал так:

assert isinstance(input_string, unicode), "Input string must  be unicode"

https://codereview.appspot.com/225420043/diff/100001/markov_gen.py#newcode156
markov_gen.py:156: for value in counter_beginners.values()]
Здесь и в chains_for_contest выполняется одно и то же действие. Почему бы не
сделать для этого одну функцию?