Коррекция работы worse_than
Issue #92
closed
Originally reported on Google Code with ID 92 ``` Валерий, я когда-нибудь научу вас что прежде чем вносить изменения в общий код, о которых вас не просили (кроме исправления явных ошибок) стоит посоветоваться?!
Изменение 7380e899494e показывает что пока с этим плохо. К тому же комментарий предельно неконкретный, т.е. вы или не разобрались в чем дело (хотя по комментариям довольно ясно) или не отметили это в сообщении.
Я поменял местами два критерия: сначала поставил минимизацию количества оставшихся символов, а потом уже - длину совпадения. Так это соответствует критериям, по которым отбирается собственно наиболее подходящий ответ из нескольких в qtype_preg_question::get_best_fit_answer Подробнее к вопросу стратегий мы еще вернемся, но пока я не вижу смысла иметь расхождения между отбором совпадений в вопросе и внутри матчера.
Если это породило фейлы, надо бы разобраться в чем дело - и что следует править, функцию или кросс-тесты. А не так вот огульно заменять код просто заметив фейлы...
Отменять коммит не надо (повезло что закомментировали, а не удалили - тем более что коммит опять, как нарочно, не атомарный), но это issue достается вам в награду.
Для начала необходимо сделать две вещи: 1) анализ фейлов - связаны они с упомянутым выше изменением порядка применения критериев или другим отличием в поведении функций? Напишите здесь перечень фейлов, что не так и чем вызвано по вашему... 2) разработать набор юнит-тестов для желаемого поведения этой функции, когда мы его оговорим ```
Reported by `oasychev` on 2012-01-09 18:18:54
Comments (13)
-
reporter
-
reporter
``` Я также не считаю $orequal устаревшим, т.к. он легко позволяет использовать ее в двух вариантах: "меньше" и "меньше или равно" (равно с точки зрения хуже/лучше, а не эквивалентности). Я бы не хотел выделять "равно" в отдельную функцию, т.к. при изменении состава критериев нужно будет не забыть отредактировать обе... ```
Reported by `oasychev` on 2012-01-09 20:22:23
-
``` При использовании вашей реализации получается странная ситуация (при перемене местами 3 и 4 проверок все нормально):
qtype_nfa_preg_matcher failed 'index_first' check on regex '((abc)\2)\1' and string 'abcabcab' obtained result Array ( [0] => 3 [1] => -1 [2] => 3 ) for 'index_first' is incorrect obtained result Array ( [0] => 5 [1] => -1 [2] => 3 ) for 'length' is incorrect
Он ведь должен с нулевого символа захватить всю эту строку, без полного совпадения... ```
Reported by `vostreltsov` on 2012-01-09 22:33:15
-
reporter
``` А какой left получается при этом? И какой left у совпадения с начала?
Насколько я понимаю, с этим выражением может совпасть только строка из 4-х abc, других вариантов нет. Поэтому частичное совпадение с начала должно иметь меньшую left и выигрывать при моих вариантах проверок тоже. Так что не факт что баг именно в worse_than, а если там - то скорее в реализации чем в порядке проверок. Разберитесь... ```
Reported by `oasychev` on 2012-01-10 07:12:18
-
``` В функцию добавлен параметр стратегии матчинга, кросс-тесты дополнены, баг в NFA матчере исправлен. ```
Reported by `vostreltsov` on 2012-01-10 08:48:50 - Status changed: `Fixed`
-
reporter
``` Я бы не торопился с фиксированием этого. Мы так и не обсудили пункты 1 и 3 из комментария 1.
Т.е. 1) нужна ли проверка на длину больше нуля? Если стратегия длины совпадения, то точно не нужна - она по длине пройдет; если минимальной длины продолжения - тогда интереснее. Что лучше, совпадение нулевой длины которое заканчивается быстрее или совпадение ненулевой длины которое заканчивается дольше? Такую ситацию можно устроить, рассматривая альтернативы, одна из которых начинается с \b
2) Унаследовать ли отдельно с подмасками для НКА ?
Ваше мнение по этим вопросам... ```
Reported by `oasychev` on 2012-01-12 16:02:38
-
``` 1) Проверку на более длинное совпадение при одинаковом количестве оставшихся символов можно как раз засунуть внутрь соответствующей ветки if'а, отвечающего за стратегию. В начале функции это излишне, как мне кажется.
2) Вообще говоря количество подмасок был workaround для специфичной ситуации с подмасками нулевой длины. Теперь это будет решаться приоритетами переходов (насколько я понял из pdf'ок), и из общего кода это точно нужно убрать. В любом случае будет унаследованный метод с вызовом родительского, в котором я буду проверять приоритеты. Если и понадобится количество подмасок, проверю это в наследнике. ```
Reported by `vostreltsov` on 2012-01-12 18:46:12
-
reporter
``` 1) критерий 2 пока остается, т.к. иначе могут "победить" варианты без is_match Вопрос, однако, в том - не следует ли заменить там проверку длины на проверку is_match?
2) 5-й критерий перепишем когда перейдем на новый формат автомата... ```
Reported by `oasychev` on 2012-01-12 21:04:43
-
reporter
``` Критерий 2 я заменил на is_match() вместо нулевой длины - новых фейлов не дает. Если не возражаете, оставляем в новой редакции.
Возможно также критерий номер 1 стоит вместо full заменить на best() (которая на данный момент эквивалентна). Ваше мнение? ```
Reported by `oasychev` on 2012-01-14 04:55:44
-
``` С is_match согласен, а с best() не совсем. Нам нужно именно из полного и неполного выбрать полное совпадение, а best() может поменяться... ```
Reported by `vostreltsov` on 2012-01-14 12:44:47
-
reporter
``` А почему мы выбираем полное первым критерием? Не потому ли, что раз встретив его уже не надо ничего искать дальше? Это и есть смысл best()... ```
Reported by `oasychev` on 2012-01-14 12:54:32
-
``` Ок. ```
Reported by `vostreltsov` on 2012-01-14 12:59:26
-
reporter
``` Пока я думаю достаточно. ```
Reported by `oasychev` on 2012-02-18 22:26:11 - Status changed: `Done`
- Log in to comment
``` Какие моменты необходимо решить глобально относительно worse_than: 1) следует ли проверять full и ненулевую длину по отдельности, или использовать is_match (примеры когда это лучше сделать по отдельности, если можно, приведите...) 2) порядок следования критериев "минимум остался" и "максимальная длина совпадения" под вопросом, сейчас сам вопрос проверяем "минимум остался" при возможности... 3) проверка количества совпавших подмасок нужна только НКА внутри match_from_pos если я правильно понимаю, но не циклу перебора позиций. Может быть сделать qtype_preg_nfa_matchin_results с проверками, специфичными для НКА? ```
Reported by `oasychev` on 2012-01-09 20:09:28