Review PRICKLE4 #25
Review PRICKLE4 #25
Conversation
| def transcribe(self): | ||
| """ | ||
| Function return transcript of DNA sequence | ||
| """ | ||
| transcribe = self.seq.maketrans(self.dict_trans) | ||
| res = self.seq.translate(transcribe) |
There was a problem hiding this comment.
Усложнили немного)
Тут можно обойтись просто заменой T на U, вместо использования отдельного алфавита под транскрипцию
There was a problem hiding this comment.
Но в целом так тоже окей)
| alphabet = {'M', 'O', 'v', 'D', 'f', 'N', 'c', 'A', 'R', 'W', 'I', 'm', 'L', 's', 'H', 'q', 'w', 'V', 'n', 'i', | ||
| 'g', 'F', 'S', 'e', 'l', 'U', 'P', 'Q', 'K', 'Y', 'u', 'y', 'd', 'h', 'k', 'r', 't', 'G', 'o', 'E', | ||
| 'p', 'T', 'C', 'a'} | ||
| masses = {'A': 71.08, 'R': 156.2, 'N': 114.1, 'D': 115.1, 'C': 103.1, 'E': 129.1, 'Q': 128.1, 'G': 57.05, 'H': 137.1, | ||
| 'I': 113.2, 'L': 113.2, 'K': 128.2, 'M': 131.2, 'F': 147.2, 'P': 97.12, 'S': 87.08, 'T': 101.1, | ||
| 'W': 186.2, 'Y': 163.2, 'V': 99.13, 'U': 168.05, 'O': 255.3, 'a': 71.08, 'r': 156.2, 'n': 114.1, 'd': 115.1, | ||
| 'c': 103.1, 'e': 129.1, 'q': 128.1, 'g': 57.05, 'h': 137.1, 'i': 113.2, 'l': 113.2, 'k': 128.2, 'm': 131.2, | ||
| 'f': 147.2, 'p': 97.12, 's': 87.08, 't': 101.1, 'w': 186.2, 'y': 163.2, 'v': 99.13, 'u': 168.05, 'o': 255.3} |
There was a problem hiding this comment.
Такое количество алфавитов кажется избыточным. Если вы хотите проверить последовательность и определить массу, вы можете использовать всего один словарь)
| """ | ||
| Function return complement sequence | ||
| """ | ||
| complementary_dna = self.seq.maketrans(self.dict_comp) |
| @abstractmethod | ||
| def to_print_seq(self): | ||
| return self.seq | ||
| pass | ||
|
|
There was a problem hiding this comment.
Абстрактные методы в абстрактном классе определяют некий интерфейс, который будет унаследован дочерними классами. Тут подразумевалось, что в родительском классе Biological sequence вы скажите, что дочерние классы должны уметь считать длину, получать индексы и прочее. Например, про длину, это можно записать как
| @abstractmethod | |
| def to_print_seq(self): | |
| return self.seq | |
| pass | |
| @abstractmethod | |
| def __len__(self): | |
| pass | |
И потом переопределить нашу заглушку в дочернем, написав что-то осмысленное вместо pass
Как-то так)
| for nucl in self.seq: | ||
| if nucl == 'c' or nucl == 'g' or nucl == 'C' or nucl == 'G': | ||
| n += 1 | ||
| return 100 * n / len(self.seq) |
There was a problem hiding this comment.
Вместо цикла можно использовать методы строк, типа
| for nucl in self.seq: | |
| if nucl == 'c' or nucl == 'g' or nucl == 'C' or nucl == 'G': | |
| n += 1 | |
| return 100 * n / len(self.seq) | |
| n = self.seq.upper().count('G') + self.seq.upper().count('C') | |
| return 100 * n / len(self.seq) | |
либо использовать regex :)
| return RNASequence(res) | ||
|
|
||
|
|
||
| class RNASequence(BiologicalSequence): |
There was a problem hiding this comment.
То же самое наследование от NucleicAcidSequence
| transcribe = self.seq.maketrans(self.dict_trans) | ||
| res = self.seq.translate(transcribe) | ||
|
|
||
| return RNASequence(res) |
There was a problem hiding this comment.
Круто, что предусмотрели, что возвращается объект другого класса!
| if not os.path.isdir('fastq_filtrator_resuls'): | ||
| os.mkdir('fastq_filtrator_resuls') |
There was a problem hiding this comment.
Папка для результатов - отличное решение)
| length_bounds_both_side = (0, length_bounds) | ||
| else: | ||
| length_bounds_both_side = length_bounds | ||
| records = list(SeqIO.parse(input_fastq, "fastq")) |
There was a problem hiding this comment.
Думаю, так тоже рабочий вариант) Или же еще можно итерироваться по объектам SeqIO, и например, скормить только последовательность в виде seq_record.seq функцию filter_lenghth, кроме того метод .letter_annotations сам по себе спокойно работает с классом SeqRecord)
| SeqIO.write((record for record in filtered_records_q), output_fastq, "fastq") | ||
| return |
There was a problem hiding this comment.
Работа хорошая, мне понравилась) что то взяла себе на заметку. Удачи!
IuriiSl
left a comment
IuriiSl
left a comment
There was a problem hiding this comment.
В целом работа хорошая. Немножко подправить абстрактные методы. Из-за путаницы в наследовании у ДНК и РНК не будет работать проверка алфавита. Есть парочка интересных решений)
| class BiologicalSequence(ABC, str): | ||
|
|
||
| def __init__(self, seq): | ||
| self.seq = seq |
There was a problem hiding this comment.
Абстрактный класс должен содержать только абстрактные методы, которые определяют, что должно быть в дочерних классах
| complementary_dna = self.seq.maketrans(self.dict_comp) | ||
| res = self.seq.translate(complementary_dna) |
| alphabet = {'U', 'A', 'g', 'G', 'a', 'c', 'C', 'u'} | ||
| dict_comp = {'A': 'U', 'C': 'G', 'U': 'A', 'G': 'C', 'a': 'u', 'c': 'g', 'u': 'a', 'g': 'c'} |
There was a problem hiding this comment.
Хорошо, что предусмотрены регистры во всех алфавитах
| res = self.seq.translate(complementary_dna) | ||
| return NucleicAcidSequence(res) | ||
|
|
||
| class DNASequence(BiologicalSequence): |
There was a problem hiding this comment.
По идее не должно работать, если бы абстрактный класс содержал абстрактные методы. И получается, что проверка алфавита отсутствует
| return RNASequence(res) | ||
|
|
||
|
|
||
| class RNASequence(BiologicalSequence): |
| def filter_gc(records, gc_bounds_both_side=(0, 100)) -> list: | ||
| """ | ||
| This function selects sequences with the GC content of your interest | ||
| :parameters: | ||
| records: records from fastq parced by SeqIO | ||
| gc_bound: interval for the of acceptable GC content, in % | ||
| :return:(dict) new dictionary consists of selected sequences | ||
| """ |
There was a problem hiding this comment.
Видимо, забыли исправить докстрингу. Функция возвращает список, а не словарь.
| for record in records: | ||
| if (length_bounds_both_side[1] >= len(record.seq) >= length_bounds_both_side[0]): | ||
| new_records.append(record) | ||
| print(new_records) |
| for record in records: | ||
| if (sum(record.letter_annotations["phred_quality"])/len(record.seq) >= quality_threshold): | ||
| new_records.append(record) | ||
| print(new_records) |
zmitserbio
left a comment
zmitserbio
left a comment
There was a problem hiding this comment.
В целом, работа неплохая, отдельные моменты отметил себе на будущее. Однако есть проблемы с не совсем корректной реализацией классов.
Еще, стоит прогнать код через линтер - периодически количество пустых строк не соответствует требуемому правилами. И лучше сделать аннотации типов.
| from Bio import SeqUtils | ||
|
|
||
|
|
||
| class BiologicalSequence(ABC, str): |
There was a problem hiding this comment.
Здесь не стоило наследоваться от строки, все же это абстрактный класс, ну и вообще, как оказалось, наследование от базового класса здесь не предполагалось.
|
|
||
| def __init__(self, seq): | ||
| self.seq = seq |
There was a problem hiding this comment.
Как сама автор кода указывает ниже, здесь стоило оставить только абстрактные методы. Тот же комментарий к методам ниже.
| # не поняла, не доделала | ||
| @abstractmethod | ||
| def to_print_seq(self): | ||
| return self.seq | ||
| pass |
There was a problem hiding this comment.
Здесь, если я правильно понимаю, предполагалась реализация "вывода на печать в удобном виде"? В таком случае, стоило это сделать через str. Вообще, как я понимаю, предполагалось, что наш объект имеет атрибут, в котором лежит строка с последовательностью. Тогда можно было бы сделать так:
| # не поняла, не доделала | |
| @abstractmethod | |
| def to_print_seq(self): | |
| return self.seq | |
| pass | |
| def __str__(self): | |
| return self.seq |
Как я понимаю, здесь логика была иной, учитывая наследование от строки. Такой объект тоже можно было бы перевести в тип строка, например, итерируясь по нему, добавляя элементы в list, а затем переведя лист в строку. Это не очень осмысленно, но если нужен именно тип строка, так можно было бы сделать.
Повторюсь только, что в абстрактном классе делать этого не стоило.
| class UnexpectedSymbolInSeqError(ValueError): | ||
| pass |
There was a problem hiding this comment.
Приятно, что сделана специальная ошибка.
| alphabet = {'M', 'O', 'v', 'D', 'f', 'N', 'c', 'A', 'R', 'W', 'I', 'm', 'L', 's', 'H', 'q', 'w', 'V', 'n', 'i', | ||
| 'g', 'F', 'S', 'e', 'l', 'U', 'P', 'Q', 'K', 'Y', 'u', 'y', 'd', 'h', 'k', 'r', 't', 'G', 'o', 'E', | ||
| 'p', 'T', 'C', 'a'} | ||
| masses = {'A': 71.08, 'R': 156.2, 'N': 114.1, 'D': 115.1, 'C': 103.1, 'E': 129.1, 'Q': 128.1, 'G': 57.05, 'H': 137.1, | ||
| 'I': 113.2, 'L': 113.2, 'K': 128.2, 'M': 131.2, 'F': 147.2, 'P': 97.12, 'S': 87.08, 'T': 101.1, | ||
| 'W': 186.2, 'Y': 163.2, 'V': 99.13, 'U': 168.05, 'O': 255.3, 'a': 71.08, 'r': 156.2, 'n': 114.1, 'd': 115.1, | ||
| 'c': 103.1, 'e': 129.1, 'q': 128.1, 'g': 57.05, 'h': 137.1, 'i': 113.2, 'l': 113.2, 'k': 128.2, 'm': 131.2, | ||
| 'f': 147.2, 'p': 97.12, 's': 87.08, 't': 101.1, 'w': 186.2, 'y': 163.2, 'v': 99.13, 'u': 168.05, 'o': 255.3} |
There was a problem hiding this comment.
Тут стоит отметить, что хотя решение не является неправильным, alphabet здесь не обязательная переменная (кстати, переменные стоит писать капсом). Ведь можно использовать set(masses.keys()). Это, конечно, не ошибка, но на будущее, это как будто несколько более экономно, не хранить избыточную информацию (особенно учитывая, что в реальных примерах она может весить гигабайты).
| :parameters: | ||
| records: records from fastq parced by SeqIO | ||
| gc_bound: interval for the of acceptable GC content, in % | ||
| :return:(dict) new dictionary consists of selected sequences |
There was a problem hiding this comment.
Следует поправить тип возвращаемого объекта.
| :parameters: | ||
| records: records from fastq parced by SeqIO | ||
| length_bound: interval for the of acceptable sequense length in number of nucleotide | ||
| :return:(dict) new dictionary consists of selected sequences |
| parameters: | ||
| seqs: dictionary of FASTQ sequences {name: (sequence, quality)} | ||
| quality_treshold: threshold value for average quality per nucleotide (phred33 scale) | ||
| :return:(dict) recordes for selected sequences |
| for record in records: | ||
| if (length_bounds_both_side[1] >= len(record.seq) >= length_bounds_both_side[0]): | ||
| new_records.append(record) | ||
| print(new_records) |
There was a problem hiding this comment.
Не очень понятно, почему здесь принт... Видимо, забыли убрать?
| for record in records: | ||
| if (sum(record.letter_annotations["phred_quality"])/len(record.seq) >= quality_threshold): | ||
| new_records.append(record) | ||
| print(new_records) |
4 participants
Review PRICKLE4