-
Notifications
You must be signed in to change notification settings - Fork 0
Review PCDHA12 #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Review PCDHA12 #27
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,121 @@ | ||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||
| from abc import ABC, abstractmethod | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import numpy as np | ||||||||||||||||||||||||||||||||||||||||
| from Bio import SeqIO | ||||||||||||||||||||||||||||||||||||||||
| from Bio.SeqRecord import SeqRecord | ||||||||||||||||||||||||||||||||||||||||
| from Bio.SeqUtils import GC | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # 2.4 | ||||||||||||||||||||||||||||||||||||||||
| def filter_length(record: SeqRecord, length_bounds: tuple) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| return length_bounds[0] <= len(record.seq) <= length_bounds[1] | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def filter_gc(record: SeqRecord, gc_bounds: tuple) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| return gc_bounds[0] <= GC(record.seq) <= gc_bounds[1] | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def filter_quality(record: SeqRecord, quality_threshold: int) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| avg_quality = np.mean(record.letter_annotations["phred_quality"]) | ||||||||||||||||||||||||||||||||||||||||
| return avg_quality >= quality_threshold | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def filter_fastq(input_path: str, | ||||||||||||||||||||||||||||||||||||||||
| gc_bounds: tuple = (0, 100), | ||||||||||||||||||||||||||||||||||||||||
| length_bounds: tuple = (0, 2 ** 32), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+24
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Всё круто! Хорошее решение делать фильтрацию, возвращая |
||||||||||||||||||||||||||||||||||||||||
| quality_threshold: int = 0) -> None: | ||||||||||||||||||||||||||||||||||||||||
| """ Reads a FASTQ file, filters sequences based on GC content, sequence | ||||||||||||||||||||||||||||||||||||||||
| length and quality threshold, and writes the filtered sequences to | ||||||||||||||||||||||||||||||||||||||||
| a new file """ | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Спасибо за докстринг! Но мне казалось, что туда необходимо добавлять, какой инпут и какой аутпут функции идет. Что-то вот такое
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| path, filename = os.path.split(input_path) | ||||||||||||||||||||||||||||||||||||||||
| name, ext = os.path.splitext(filename) | ||||||||||||||||||||||||||||||||||||||||
| output_path = path + "/" + f"{name}_filtered{ext}" | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. мне нравится способ генерации имени выходного файла, здесь также можно было учесть из тз, что пользователь сам тоже может задавать имя файла. |
||||||||||||||||||||||||||||||||||||||||
| input_seq_iterator = SeqIO.parse(input_path, 'fastq') | ||||||||||||||||||||||||||||||||||||||||
| filtered_seq_iterator = (record for record in input_seq_iterator | ||||||||||||||||||||||||||||||||||||||||
| if filter_length(record, length_bounds) | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. отличный способ проверить все три условия и сразу записать, list comprehension тут лаконично и эффективно вписался. то, что функции возвращают bool позволяет его провести- это хороший подход |
||||||||||||||||||||||||||||||||||||||||
| and filter_gc(record, gc_bounds) | ||||||||||||||||||||||||||||||||||||||||
| and filter_quality(record, quality_threshold)) | ||||||||||||||||||||||||||||||||||||||||
| SeqIO.write(filtered_seq_iterator, output_path, "fastq") | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # 2.5 | ||||||||||||||||||||||||||||||||||||||||
| class BiologicalSequence(ABC, str): | ||||||||||||||||||||||||||||||||||||||||
| @abstractmethod | ||||||||||||||||||||||||||||||||||||||||
| def check_alphabet(self) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. В вашем абстрактном классе не реализованы еще сет методов: работа c |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Здесь надо было добавить еще абстрактных методов, но об этом уже сказали :) (Прости, я вижу чужое ревью, только когда проверяю)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Все нормально! Нет ничего плохого в том чтобы повторять уже сказанные моменты, так как в первую очередь вам надо делать свое ревью. |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class NucleicAcidSequence(BiologicalSequence): | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Не хватает докстринг в классах |
||||||||||||||||||||||||||||||||||||||||
| def __init__(self, sequence): | ||||||||||||||||||||||||||||||||||||||||
| raise NotImplementedError('An instance of this class cannot be created') | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Здесь можно было бы при ините делать последовательность сразу большими буквами, чтобы потом в словаре не хранить и маленькие, и большие буквы. И в целом инит вынести в этот класс, и убрать его в дочерних, с точки зрения информации подаваемой, что АА, что РНК, что ДНК это все строчки с буковками. Так что я бы вместо ошибки добавила вот это, а потом от этого дальше наследовалась, чтобы не дублироваться.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def check_alphabet(self) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| """Checks if the provided nucleic acid sequence contains | ||||||||||||||||||||||||||||||||||||||||
| only acceptable letters""" | ||||||||||||||||||||||||||||||||||||||||
| sequence_chars = set(self.sequence) | ||||||||||||||||||||||||||||||||||||||||
| return sequence_chars.issubset(type(self).ALPHABET) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def complement(self): | ||||||||||||||||||||||||||||||||||||||||
| """Complements DNA or RNA sequence.""" | ||||||||||||||||||||||||||||||||||||||||
| complement_seq = ''.join(type(self).COMPLEMENT_DICT.get(base) | ||||||||||||||||||||||||||||||||||||||||
| for base in self.sequence) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+61
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Интересная конструкция) Возьму на заметку |
||||||||||||||||||||||||||||||||||||||||
| return type(self)(complement_seq) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def get_gc(self) -> float: | ||||||||||||||||||||||||||||||||||||||||
| """Calculates GC content for nucleic acid sequence""" | ||||||||||||||||||||||||||||||||||||||||
| return GC(self.sequence) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class RNASequence(NucleicAcidSequence): | ||||||||||||||||||||||||||||||||||||||||
| ALPHABET = ('A', 'U', 'G', 'C', 'N') | ||||||||||||||||||||||||||||||||||||||||
| COMPLEMENT_DICT = {'A': 'U', 'U': 'A', 'G': 'C', 'C': 'G', 'N': 'N', | ||||||||||||||||||||||||||||||||||||||||
| 'a': 'u', 'u': 'a', 'g': 'c', 'c': 'g'} | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+70
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Здорово! Выносить эти словари не в инит, по моему мнению очень разумно, как и не выносить их глобально за пределы классов.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. А еще теперь это классовые атрибуты, поэтому их не надо делать капсом There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. да, верно, словарь лучше вынести за функцию. помимо этого, мне нравится логика наследования методов и полиморфизм здесь: то есть методы проверки на ДНК/РНК и построения комплементарной цепи реализуются для каждого дочернего класса называются одинаково, но учитывают отличия дня/рнк |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __init__(self, sequence): | ||||||||||||||||||||||||||||||||||||||||
| self.sequence = sequence | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class DNASequence(NucleicAcidSequence): | ||||||||||||||||||||||||||||||||||||||||
| ALPHABET = ('A', 'T', 'G', 'C', 'N') | ||||||||||||||||||||||||||||||||||||||||
| COMPLEMENT_DICT = {'A': 'T', 'T': 'A', 'G': 'C', 'C': 'G', 'N': 'N', | ||||||||||||||||||||||||||||||||||||||||
| 'a': 't', 't': 'a', 'g': 'c', 'c': 'g', 'n': 'n'} | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __init__(self, sequence): | ||||||||||||||||||||||||||||||||||||||||
| self.sequence = sequence | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def transcribe(self) -> RNASequence: | ||||||||||||||||||||||||||||||||||||||||
| """Transcribes DNA sequence into RNA sequence. | ||||||||||||||||||||||||||||||||||||||||
| Returns RMASequence object""" | ||||||||||||||||||||||||||||||||||||||||
| return RNASequence(self.sequence.replace('T', 'U').replace('t', 'u')) | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. я также реализовала транскрипцию, но оказалось это не самый оптимальный вариант, так как двойной replace = двойной проход по последовательности. молодец, что возвращаешь экземпляр класса RNASeq |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| class AminoAcidSequence(BiologicalSequence): | ||||||||||||||||||||||||||||||||||||||||
| ONE_LETTER_ALPHABET = ('A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K', 'L', | ||||||||||||||||||||||||||||||||||||||||
| 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'Y') | ||||||||||||||||||||||||||||||||||||||||
| THREE_LETTER_ALPHABET = ('Ala', 'Cys', 'Asp', 'Glu', 'Phe', 'Gly', 'His', | ||||||||||||||||||||||||||||||||||||||||
| 'Ile', 'Lys', 'Leu', 'Met', 'Pro', 'Gln', 'Arg', | ||||||||||||||||||||||||||||||||||||||||
| 'Ser', 'Thr', 'Val', 'Trp', 'Tyr') | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+94
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Круто, что предусмотрено два варианта алфавита |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def __init__(self, sequence: str): | ||||||||||||||||||||||||||||||||||||||||
| self.sequence = sequence | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def check_alphabet(self) -> bool: | ||||||||||||||||||||||||||||||||||||||||
| """Checks if the provided amino acid sequence contains only | ||||||||||||||||||||||||||||||||||||||||
| acceptable letters in one-letter or three-letter code""" | ||||||||||||||||||||||||||||||||||||||||
| sequence_chars = set(self.sequence) | ||||||||||||||||||||||||||||||||||||||||
| return (sequence_chars.issubset(self.ONE_LETTER_ALPHABET) | ||||||||||||||||||||||||||||||||||||||||
| or sequence_chars.issubset(self.THREE_LETTER_ALPHABET)) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def get_molecular_weight(self) -> float: | ||||||||||||||||||||||||||||||||||||||||
| """ calculate molecular weight for one-letter amino acid sequence""" | ||||||||||||||||||||||||||||||||||||||||
| WEIGHT_DICT = { | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. насколько мне известно, словарь лучше вынести за функцию |
||||||||||||||||||||||||||||||||||||||||
| 'G': 57.051, 'A': 71.078, 'S': 87.077, 'P': 97.115, 'V': 99.131, | ||||||||||||||||||||||||||||||||||||||||
| 'T': 101.104, 'C': 103.143, 'I': 113.158, 'L': 113.158, 'N': 114.103, | ||||||||||||||||||||||||||||||||||||||||
| 'D': 115.087, 'Q': 128.129, 'K': 128.172, 'E': 129.114, 'M': 131.196, | ||||||||||||||||||||||||||||||||||||||||
| 'H': 137.139, 'F': 147.174, 'R': 156.186, 'Y': 163.173, 'W': 186.210 | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| terminal_h_oh_weight = 18.02 | ||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. круто, даже масса терминальной hoh учтена) |
||||||||||||||||||||||||||||||||||||||||
| weight = (terminal_h_oh_weight + | ||||||||||||||||||||||||||||||||||||||||
| sum(WEIGHT_DICT[aa] for aa in self.sequence if aa in WEIGHT_DICT)) | ||||||||||||||||||||||||||||||||||||||||
| return weight | ||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
мне очень нравится реализация этих функций: кратко и понятно (есть аннотация с каждому аргументу, почти все уместилось на одну строку, без лишних переменных)
однако я помню, что по заданию границы для длины и GC состава могут быть заданы как в виде одного числа, так и в виде интервала. в ваших функциях есть только интервал