ugeneunipro / ugene

UGENE is free open-source cross-platform bioinformatics software
http://ugene.net
GNU General Public License v2.0
207 stars 60 forks source link

UGENE-8101 Add "Exclude enzymes found in the exclude region" feature #1632

Closed rasputinkirill closed 1 month ago

yalgaer commented 3 months ago

Это плохое техническое решение и вот почему.

На данный момент диалог работы с энзимами и так переусложнен: на первый план вынесен редко нужный функционал, который заслоняет то, что нужно всегда.

То, как в этом диалоге развивается выбор региона - это хороший пример, когда делается минимальное изменение вместо правильного (возможно большего по размеру), что приводит в итоге к плохому результату типа "Exclude enzymes found in exclude region".

image

Как сделать правильно: вместо того, чтобы добавлять все новые и новые одноразовые (используемые только здесь) опции выбора региона нужно правильно использовать идею region selector widget:

В region selector есть опции: 1 Вся последовательность 2 Custom region 3 Selected region

Selected регион в данном случае может быть сложным (список регионов) так как он может браться из покрытия любой аннотации: выделил аннотацию -> получил selected region.

Более того, selected region опцию в region selector можно развить, когда диапазон задается руками: в виде location string - это тоже будет multi-region, уже без необходимости использования аннотации.

Итого:

  1. Направление решение выбрано плохо с точки зрения UI (биологическую часть я не критикую)
  2. Нужно правильно использовать/доразвить region selector и от этого будет выгода всем местам где он используется в UGENE.
yalgaer commented 2 months ago

Закрываю до обновления

rasputinkirill commented 2 months ago

@yalgaer Переделал гуи, посмотри устраивает или нет. Есть еще проблемы которые попаравлю - трансляции, гуи тест, возможно xml тесты придется переписать. Вопрос: должно ли наличие селекшена в сиквенсе как то учитываться создании диалога, то есть должен ли автоматически координаты селекшена заполнить регион поиска. Потому что сейчас мы заполняем настройки запомненными данными из прошлого запуска таски дял этого сиквенс обджекта (либо значениями по умолчанию). Должны ли координаты селекшена "перебить" эти координаты или нет? Либо вообще игнорируем наличие селекшена на сиквенсе?

yalgaer commented 2 months ago

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

Я бы сделала это одной из опций регион-селектора: если есть selection то показывать опцию "Selection" которая не модифицируется пользователем напрямую

rasputinkirill commented 2 months ago

Переделал. Добавил опцию LOCATION в текущий виджет. Пойдет так? Как поступаем теперь с новым функционалом, добавляем еще один такой виджет с чекбоксом Excleded?

yalgaer commented 2 months ago

Переделал. Добавил опцию LOCATION в текущий виджет. Пойдет так?

Начинаю смотреть, добавлю ревью в течение дня.

Как поступаем теперь с новым функционалом, добавляем еще один такой виджет с чекбоксом Excleded?

А почему одного виджета не хватит: пусть выберут формат с ручным вводом location и напишут там любой join?

rasputinkirill commented 2 months ago

А почему одного виджета не хватит: пусть выберут формат с ручным вводом location и напишут там любой join?

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

yalgaer commented 2 months ago

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

Именно, что обсуждали: если в последовательности 1..1000 надо исключить энзимы располагающиеся в регионе 100.200 то достаточно используя region selector задать location: 1..100,200..1000

Возможно, я не помню какой-то другой вариант, который на самом деле невозможен, так как задача сильно затянулась по времени. Напомни, пожалуйста.

rasputinkirill commented 2 months ago

Именно, что обсуждали: если в последовательности 1..1000 надо исключить энзимы располагающиеся в регионе 100.200 то достаточно используя region selector задать location: 1..100,200..1000

Например если у нас одно поле ввода формата genbank location и мы хотим поискать энзимы в двух регионах 1..100,200..300, но исключить из результатов энзимы найденные в 50..70 то мы не сможем закодировать это в одной строке. А если регионы указаны не с начала последовательности и не до конца последовательности как поступать с этими кусками? Мы не можем в таком случае указать хотим мы исключать найденные там энзимы или это области где вообще не производить поиск.

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

Опять же повторю, если бы у нас не было нового функционала "исключить энзимы найденные в указанном(ых) регионе(ах)". Мы могли бы использовать всего лишь одно поле ввода в котором в формате genbank location можно указать раздробленную область поиска. То теперь в случае применения новой фичи мы должны указать область(и) исключения энзимов найденных в них.

DmitriiSukhomlinov commented 2 months ago

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

Основная идея региона Exclude состоит в том, чтобы подбирать сайты рестрикции вне некоего фрагмента. Это нужно, если у нас есть некая последовательность, которую мы хотим проклонировать (назовем её целевой - собственно, так её обычно и называют), которая является частью другой, большой последовательности и для проведения реакции нам нужно целевую последовательность вырезать из большой. Поскольку настолько маленьких ножниц еще не изобрели, для решения этой задачи пользуются сайтами рестрикции. Сайт рестрикции представляет из себя короткую последовательность, на которую налипают определенные ферменты и режут сайт в заранее известном месте.

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

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

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

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

Теперь, когда нам всем понятно, что и зачем происходит, первым делом стоит задать вопрос - зачем вообще мы изменяем графический интерфейс?

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

Во-вторых, изменение графического интерфейса никаким образом не улучшает его с точки зрения данной задачи, а наоборот, ухудшает. Допустим, у нас есть последлвательность длиной 100 нукледотидов, мы выбираем в качестве региона поиска 20-80, а в качестве региона Exclude - 30-70. Текущий интерфес подразумевает, что, в контекстве данной функции, у нас есть три типа регионов:

  1. вообще не участвующие в поиске (0-20 и 80-100);
  2. те, где мы ищем сайты рестрикции (20-30 и 70-80);
  3. и Exclude регион, то бишь целевая последовательность, где мы ищем сайты, которые мы исключим из результата (30-70).

Изменение графического интерфеса объединяет группы 1 и 3 и становится не понятно, каким образом их отличить.

Поэтому, я предлагаю следующее:

rasputinkirill commented 2 months ago

Делаю такую логику: если выбрана опция задания региона поиска genbank location и она содержит несколько регионов, то не включенные между самой левой и самой правой координатой рассматриваем как места энзимы из которых исключаются. Старую опцию Exclude выкидываем из алгоритма, работаем только с новой. Я правильно понял?

yalgaer commented 2 months ago

Делаю такую логику: если выбрана опция задания региона поиска genbank location и она содержит несколько регионов, то не включенные между самой левой и самой правой координатой рассматриваем как места энзимы из которых исключаются. Старую опцию Exclude выкидываем из алгоритма, работаем только с новой. Я правильно понял?

Я кажется поняла, что было проблемой моего понимания текущего UI.

Если мы нашли на промежутке 1...100 энзимы A (1), B(2), B(50), A(98) и B(99) и хотим быть уверены что у нас в результате разреза сохранится регион 10..90 то в результатах не должно быть B, так как он может отрезать по позиции 50.

Думаю та же проблема с пониманием UI есть и у пользователей и нужно придумать как сделать UI прозрачным.

Поэтому соглашусь, что второй region selector нужен.

Его стоит назвать [x] Uncut Region и добавить тултип о том что это значит:

"A region that will not be cut by any of the found enzymes. If an enzyme is present in this region, it will be excluded from the flank results."

Что делать с алгоритмом: исправить его - он написан неверно. Ошибка в логике пошла еще с этого фикса: https://ugene.dev/tracker/browse/UGENE-3267 который обрабатывал exclude region неверно и стал причиной дискуссии сейчас.

rasputinkirill commented 1 month ago

Часть замечаний поправил, часть откомментировал. Найденную проблему с поиском поправил. Пишу тесты.

rasputinkirill commented 1 month ago

Вроде все замечания поправил. Только у меня почемуто трансляции не сработали хотя в лингвисте все зеленое, номера строчек вроде верные.

yalgaer commented 1 month ago

Вроде все замечания поправил. Только у меня почемуто трансляции не сработали хотя в лингвисте все зеленое, номера строчек вроде верные.

У тебя фильтрацией exclude энзимов занимается FindEnzymesToAnnotationsTask. А все что он должен делать - это перекладывать результаты поиска энзимов в форму аннотаций.

Внеси фильтрацию согласно exclude правилам в FindSingle/MultipleEnzymeTask. Как это сделать подробнее описано выше.

yalgaer commented 1 month ago

Вроде все замечания поправил. Только у меня почемуто трансляции не сработали хотя в лингвисте все зеленое, номера строчек вроде верные.

Внешне выглядит все правильно. Можно попробовать продебажить и посмотреть почему строка которая пристутствует в qm файле в итоге не заменяется на нужную (переведенную)

yalgaer commented 1 month ago

Можно перезапустить XML мак и винду чтобы были зелеными?