Рефакторинг анализаторов

[Oleg Sychev]

Originally reported on Google Code with ID 206

В основном этой задачей буду заниматься я, но некоторую начальную работу надо проделать
Дмитрию.

1 этап) унифицировать конструкторы анализаторов, прежде всего сделать lexical_analyzer
похожим на все остальные - для чего вынести полексемное сравнение на полную эквивалентность
в функцию класса вопроса (и 2-3 юнит-теста к ней сделать) и создание объектов языка/строки/пары
строк тоже - сделать там специальную функцию, типа analyze(...) - Мамонтов (как сделаете,
перенаправляйте на меня)
2) проектирование байпассов и дальнейшая унификация конструкторов - Сычев
3) абстрактный анализатор - Сычев

Reported by oasychev on 2013-07-02 14:40:35

[Former user]

Сделал первый этап

Reported by mamontov.dp on 2013-07-19 08:22:51

[Oleg Sychev]

Нарушен принцип независимости плагинов: блок ничего про LCS знать не должен. Все, что
касается порядка лексем - это проблема вопроса. Блок занимается общей работой по лексическому
анализу, анализу опечаток и т.д.

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

Reported by oasychev on 2013-07-19 10:20:11

[Oleg Sychev]

Абстрактный анализатор вытолкнул. Теперь вам работа (для начала конечно разберитесь
с интерфейсом класса, если есть вопросы - спросите) - переводить вопрос и анализаторы
на новый интерфейс.
1) в типе вопроса extra_question_fields должна собрать поля со всех анализаторов
2) в форме необходимо организовать вызов функций анализаторов для создания отдельных
секций на каждый (а также препроцессинга и валидации их параметров, не считая floatfields
которые после definition_inner мержатся в общий массив и обрабатываются общим кодом).
Каждая секция должна содержать галочку типа "использовать анализатор" (или комбу "да/нет")
со стандартным именем от поля анализатора, и поля от анализатора.
Проблема тут в том, что родительский вызов заканчивает общую секцию вопроса и добавляет
еще секцию ответов и т.д., а секции анализаторов хотелось бы разместить между общей
и ответами.
Также надо будет выделить отдельную секцию настройки хинтов, а вот язык оставляем в
общей секции вопроса - так юзеру понятнее, да и сканнинг делается даже если анализатор
Бирюковой отключен.
3) Классы самих анализаторов привести в соответствие интерфейсу. Байпасс - это ситуация,
когда данные анализ не нужен. Но (например) анализатор Бирюковой (гляньте на тип вопроса,
я предлагаю его переименовать) все равно должен заполнить correctedstring и точные
пары соответствий, чтобы потом мог нормально работать sequence_analyzer.
4) В классе вопроса написать функцию, которая запускает цепочку анализаторов (которые
активны по настройкам), передавая данные. Каждый анализатор возвращает массив измененных
string_pair и соответствующий ему массив из наборов ошибок к каждому варианту. Потом
для каждого из них запускается следующий и т.д.; при этом из них выбирается лучший.
У меня такое подозрения, что т.к. число циклов зависимо от числа анализаторов, функция
выйдет рекурсивной...

Reported by oasychev on 2013-09-08 12:58:04

[Oleg Sychev]

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

Reported by oasychev on 2013-09-08 13:11:38

[Oleg Sychev]

Элементы на форму можно вставить между уже существующими, для этого вместо addElement
надо использовать createElement, а затем insertElementBefore

Reported by oasychev on 2013-09-09 19:46:53

[Oleg Sychev]

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

Большинство из этого не самое главное, сначала надо подогнать под новый интерфейс сами
анализаторы. И решить насчет ошибок наличия лексем - как их делить между лексическим
анализатором и анализатором последовательности.

Давайте встретимся опять в среду - я уточню время чуть позже - и сделайте уже что-нибудь
к встрече - статью, скрипт для сбора данных, мелкие правки; рефакторинг попробуйте
начать - вопросы появятся... (я так понимаю на вторник вы готовы сами доложиться по
НИР, помощь научного руководителя вам не нужна).

Reported by oasychev on 2013-09-14 14:33:38

[Oleg Sychev]

Это сдерживает многие другие доработки, так что надо решить в первую очередь.

Reported by oasychev on 2013-09-14 14:34:13 - Labels added: Priority-High - Labels removed: Priority-Medium

[Oleg Sychev]

Дмитрий, что вы там за новый класс сотворили со странными функциями типа perform_deep_analysis
и еще более непонятными комментариями (из коммента к этой я вообще ничего не понял)?

Такие вещи обсуждать надо! На встрече почему ничего не говорили про такие планы?

Напишите здесь по русски как вы планируете, чтобы это работало... Комментарии ваши
трудно пока понять. И зачем это именно объект - тоже...

Reported by oasychev on 2013-09-26 21:28:37

[Former user]

Это класс служит нескольким целям
1) Он служит заменой matchedanalyzer, сохраняя результаты анализа ответа студента и
преподавателя и предоставляя методы, которые раньше предоставлял lexical_analyzer для
классов вопроса и рендерера. Таким образом снижается общая трудоемкость рефакторинга
(не надо переписывать check_match_answer и куски другого кода).

2)При этом, при создании объекта данного класса в случае полного совпадения ответов
анализа не происходит, а ответы остаются как есть. Однако, если нам нужен комплексный
анализ (perform_deep_analysis занимается этим) - мы проводим сканирование depth-first
всех комбинаций анализаторов. Я думаю не использовать рекурсию и сейчас думаю о том,
как не строить все  дерево, так как нам на каждом уровне анализаторов чаще нужны предыдущие
результаты.

Reported by mamontov.dp on 2013-09-27 05:01:42

[Former user]

Хотя, в принципе, пока у нас всего четыре анализатор, вложенность не будет большой,
так что скорее всего переполнение нам не грозит...

Reported by mamontov.dp on 2013-09-27 14:33:21

[Oleg Sychev]

Ну надо к последней функции более подробный комментарий написать и без сокращений, которые
неясно как именно трактовать.

Учтите - при рефакторинге основной целью должна быть хорошая структура кода. "Сокращение
объема рефакторинга" не лучшая цель, т.к. короче всего его не производить вообще -
юзер не заметит. Сокращать объем в ущерб структуре кода - опасно именно при рефакторинге.

Переполнение при обходе дерева нам может грозить по части ширины дерева (неплохо бы
установить настраиваемое в конфиге ограничение на количество вариантов, возвращаемых
из анализатора); но не по части глубины дерева - цепочка анализаторов очень большой
не станет.

Но мне странной кажется выбор для этого класса, и тем более с такими полями. ИМХО с
тем же почти успехом можно было сделать пару функций в класс вопроса. В частности,
я не понимаю protected $stringpair = null;  Что это за пара? По идее каждый анализатор
получает на вход пару и возвращает массив результирующих пар (так дерево и растет);
а какая именно пара будет хранится в этом вашем поле?

Reported by oasychev on 2013-09-27 19:35:26

[Former user]

Комментарии я  перепишу. Это действительно было написано быстро и вышло неудачно. Однако,
в нашем случае, я сомневаюсь, что структура кода сильно много потеряет, хотя бы потому
что в противном случае можно испортить структуру кода в question, а она все таки проверена
более чем годом работы. Кроме того, question уже и так забит методами, связанные с
интеграцией в СДО, зачем плодить ответственности у класса? 

Ну, с шириной - это надо индивидуально решать. Пока я стараюсь идти в алгоритме скорее
в глубину, чем в ширину, так что атака может вестись исключительно на число созданных
строк. Единственный анализатор, который может такое число породить - анализатор Клевцова,
но я не уверен, что мы так сразу таких объемов достигнем, тем более он обещал сделать
ограничение на число перечислений.

Мне выбор для этого класса лишним не кажется. Как я уже сказал, мне не очень нравится
переделывать renderer и qtype_correctwriting_question процентов на 70, только потому
что у нас отсутствует понятие "результатов анализа", которые кешируются. Раньше это
понятие было совмещено с лексическим анализатором и в целом он хранил всю информацию
нужную остальным от результатов. Теперь его убрали, а о том что это сломается никто
и не подумал. Почему именно "результаты", а не "анализатор"? Потому что тогда бы пришлось
делать наследование абстрактного анализатора и появлялось много вопросов философского
рода и мало рефакторинга.

Более, где надо -  стоят нужные аннотации - по которым можно все-таки проследить структуру
кода. 

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

protected $stringpair = null - это стартовая пара, с которой начинается вся цепочка
анализа. Если вам оно так не нравится, я могу передавать его параметром в perform_deep_analysis.

Reported by mamontov.dp on 2013-09-27 20:07:39

[Oleg Sychev]

Если вам нужен объект для кеширования результатов, то нужно создавать пару из stringpair
и mistakes - или, лучше, внести mistakes в stringpair как поле и кешировать последний.
Объект для кеширования надо подобрать, вопрос только что это такое и должен ли именно
он включать алгоритм анализа...

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

stringpair как поле не то чтобы мне не нравится как таковое, просто остальное - вообще
в основном параметры сравнения (не считая equal), а понять класс для результатов можно
прежде всего анализируя поля для их хранения - пока не видя адекватного набора полей
я и не вижу смысла в классе; возможно при каких-то других полях для этого результата
в процессе его обработке я бы с ним и согласился. Но вообще я бы состоянием сделал
stringpair, а сам код анализа разместил в question.

Reported by oasychev on 2013-09-27 20:56:44

[Former user]

Хранить mistakes в stringpair вообще очень и очень некрасиво. Вы очень часто говорили,
что пара строк не должна знать ничего об ошибках - а значит его там хранить нельзя.
Вообще stringpair хорош, как простая структура, "прокачиваемая" через анализаторы -
насколько помню, это его изначальная цель и она с ошибками не соотносится никак.

Это не алгоритм оценивания, а общий алгоритм сравнения ответа преподавателя с ответом
студента, инкапсулирующий работу с последовательностью анализаторов, не путайте. Или
декомпозиция уже не важна?  Его может иметь смысл выделить в отдельный класс, чтобы
иметь возможность подменить, но вот в оформлении его как методов, я очень сильно не
уверен. И вот почему. Я уже написал первое приближение к решению и у меня вышло довольно
много строк для двух методов (у меня все же было три), порядка 250+. Я говорю приближение,
потому что еще не тестировал и там возможны ошибки. В question уже 650+ строк в которых,
я сомневаюсь, кому-то захочется разбираться. Лазить по классу с кучей методов занятие
не из приятных, даже с инструментарием.

Проблему о которой вы говорите (юнит-тестируемость) имеет смысл решать более умным
способом, а именно через mocks, благо они есть в PHPUnit. Ничего не мешает подменить
добрую половину методов тем, что нам нужно. Единственный минус - это присутствие get_analyzers
в qtype. Это действительно мешает, но это не проблема этого класса и тем более не может
служить хорошим аргументом против него.  

В случае данного класса ключевую роль играет не набор полей, а методы-аксессоры, которые
он предоставляет остальным классам. Во-первых, сделав подобную пару, как <строка, ошибки>,
мы тем самым уберем информацию обо всех анализаторах, что у нас были, что не есть хорошо,
как для дебага, так и для понимания.  Поле, собственно, для результатов там одно, кроме
$equal.

Reported by mamontov.dp on 2013-09-27 21:27:17

[Oleg Sychev]

Подробно похоже надо встречаться и говорить; пока кратко скажу - про то, что stringpair
не должен ничего знать о mistakes я говорил когда у нас был только block_formal_langs_string_pair.
Его это действительно не касается. А вот qtype_correctwriting_string_pair уже другая
история, он таких ограничений не имеет. Он расширяет пару строк для всего, что должен
знать вопрос - к этому можно отнести и ошибки.

Подменять именно метод вызова анализаторов я слабо вижу смысл; потому что его роль
вызвать их обходом дерева в нужном порядке и с учетом того, какие пропустить. Не представляю,
на что это может понадобится подменять.... Подменять может иметь смысл сами анализаторы,
которые хранят алгоритмы смысловой обработки, но никак не технический метод.

Наличие get_analyzers в qtype - никакой не минус и ничему не мешает; объект qtype можно
создать всегда без всяких параметров. Он по сути инкапсулирует функции, которые должны
бы быть статическими в question. Посмотрите на код Preg который использует аналогичную
манеру как с матчерами, так и с нотациями.

Reported by oasychev on 2013-09-27 21:34:45

[Former user]

Ну, вообще  в данной строковой паре единственная  логичная причина - это если анализатору
из дочерней вершины нужно подменить/удалить ошибки анализатора верхней вершины. Пока
в абстрактном анализаторе правда это вообще невозможно, поэтому данная причина не логична.
А вопросу как раз нужно знать ошибки, но даже из имени string_pair того, что они там
хранятся не следует. Там и так три строки, так что это скорее string_triplet а не pair
:) Но это - уже проблема, потому что уже приходится объяснять, что такое processed_string.
Вам это нужно? 

Я тоже слабо вижу смысл, но это все же не просто обход дерева. Там уже реализован свой
метод  удаления ошибок и я вынес его туда, чтобы abstract_analyzer не менялся.
Я думаю, что вот в случае оптимизации - можно будет попробовать подменить.

Если get_analyzers не мешает, тогда я тем более не понимаю ваш комментарий с юнит-тестированием.

Reported by mamontov.dp on 2013-09-27 21:49:38

[Oleg Sychev]

Кстати, при оценивании с точки зрения производительности возможно сначала следует сравнить
все (!) ответы на точное совпадение - такой анализ имеет линейное время - и только
если его не найдено, начинать вызывать анализаторы для глубокого анализа ответов.

Reported by oasychev on 2013-10-03 16:45:56

[Oleg Sychev]

Так что думаете насчет моего предложения в комменте 20 по скорости оценивания?

Reported by oasychev on 2014-02-26 20:35:56

[Former user]

По-моему так сейчас и делается, просто через анализаторы. Так что можно.

Reported by mamontov.dp on 2014-02-27 04:04:47

[Oleg Sychev]

А не получается сейчас так, что если первый же анализатор не нашел точного совпадения,
то он принимается искать частичное с ошибками? А только потом уже вызывается анализатор
для второго ответа и т.д.

Reported by oasychev on 2014-02-27 10:55:49

[Former user]

Принимается. Но оценка все равно выставляется как надо.

Reported by mamontov.dp on 2014-02-27 13:21:59

[Oleg Sychev]

Я говорю о производительности. Получается, что при наличии точного совпадения с третьим
ответом мы тратим время на вычисление приближенного с первыми двумя. Не лучше сначала
отдельно от анализаторов проверить все на точное совпадение по лексемам?

Reported by oasychev on 2014-02-27 13:23:08

[Former user]

Лучше, естественно. 

Reported by mamontov.dp on 2014-02-27 13:24:33

[Oleg Sychev]

Ну так что, мы замечание в комменте номер 23 исправили или нет?

Reported by oasychev on 2014-07-10 18:18:40

[Oleg Sychev]

Дмитрий, отпишись по поводу https://code.google.com/p/oasychev-moodle-plugins/issues/detail?id=206#c23

Reported by oasychev on 2015-03-01 22:44:16

[Oleg Sychev]

Ждем ответа...

[Dmitry Mamontov]

Не исправлял, не считаю приоритетным. Хорошо, посмотрю. Уточните, пожалуйста в issue алгоритм. И попробуйте поставить меня через собаку в mention. Ни одно из ваших замечаний не пришло на e-mail. Это грустно, так как это недоработка этого трекера, но что поделать.

[Oleg Sychev]

Странно, вы assignee - уж он то должен получать... Попробуйте нажать на watch.

По сути я имел ввиду такое предложение: " Кстати, при оценивании с точки зрения производительности возможно сначала следует сравнить все (!) ответы на точное совпадение - такой анализ имеет линейное время - и только если его не найдено, начинать вызывать анализаторы для глубокого анализа ответов. "

[Dmitry Mamontov]

Ок, посмотрю. Планирую сделать полексемное сравнение. Отметил себе.

[Dmitry Mamontov]

Сделано в https://bitbucket.org/dmitry_mamontov/moodle-plugins-correctwriting-release-2016-rc

[Dmitry Mamontov]

Предлагаю закрыть - последняя правка по задаче сделана.

[Oleg Sychev]

Сделано