-
Notifications
You must be signed in to change notification settings - Fork 0
Review RHNO1 #34
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 RHNO1 #34
Conversation
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.
Привет! Хороший код! Структурированный и хорошо читаемый
Есть только пара косметических комментариев
if min(record.letter_annotations["phred_quality"]) < self.min_quality: | ||
return False | ||
|
||
gc_content = gc_fraction(record.seq) |
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_content = GC(record.seq)
Для подсчета гц-состава можно использовать такой подход. Импортируется через from Bio.SeqUtils import GC
|
||
|
||
class NucleicAcidSequence(BiologicalSequence): | ||
complement_dict = {"A": "", "T": "", "G": "", "C": ""} |
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.
Не очень поняла зачем здесь эта строка. Кажется, что она определяет словарь, который фактически не используется в классе NucleicAcidSequence, так как метод complement() использует словарь, определенный в дочерних классах DNASequence и RNASequence. Но лучше, конечно, чтобы в этом классе все определялось, а не в наследуемых.
return self.sequence | ||
|
||
def check_alphabet(self): | ||
valid_alphabet = set("ATGC") |
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.
Вижу проверку на ДНК и на аминокислоты ниже, круто! Но не вижу на РНК, лучше ее тоже добавить
return self.__class__(sequence) | ||
|
||
def gc_content(self): | ||
gc_count = sum(base in "GCgc" for base in self.sequence) |
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.
Круто, что проверяется регистр! Можно для проверки алфавита тоже такое добавить
def __str__(self): | ||
return self.sequence | ||
|
||
def check_alphabet(self): |
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.
Можно еще прописать вывод ошибок, если подаются символы, отличные от ДНК/РНК/аминокислот, без них у меня работает вообще с любой последовательностью.
|
||
def filter_fastq(self): | ||
with open(self.output_file, "w") as output_handle: | ||
for record in SeqIO.parse(self.input_file, "fastq"): |
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.
тут можно было бы добавить тест на существование файла с последовательностью, который берется на вход, если его нет, то можно выводить ошибку о его отсутствии
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.
for record in SeqIO.parse(self.input_file, "fastq"): | |
for record in SeqIO.parse(self.input_file, "fastq"): | |
if not self.input_file(): | |
raise FileNotFoundError(f"Файл '{self.input_file}' не найден!") |
self.min_length = min_length | ||
self.min_quality = min_quality | ||
self.min_gc = min_gc | ||
self.max_gc = max_gc |
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.
все логично и понятно, что приходит на вход, хорошо, что прописаны автоматические минимальные значения
|
||
|
||
class NucleicAcidSequence(BiologicalSequence): | ||
complement_dict = {"A": "", "T": "", "G": "", "C": ""} |
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.
не совсем понятно для чего этот словарь, если я правильно понимаю, он дальше меняется, но вероятно, тогда стоит задать пустой словарь
|
||
def check_alphabet(self): | ||
valid_alphabet = set("ATGC") | ||
return set(self.sequence) <= valid_alphabet |
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.
хорошая проверка, на всякий случай можно добавить в сет значения с нижним регистром, или добавить при вводе последовательности перевод все в верхний регистр
|
||
|
||
class DNASequence(NucleicAcidSequence): | ||
complement_dict = {"A": "T", "T": "A", "G": "C", "C": "G"} |
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.
Вероятно этот комментарий супер вкусовщина. Более приятно читается словарь, когда он построчный. Но это сугубо вкусовщина
complement_dict = {"A": "T", "T": "A", "G": "C", "C": "G"} | |
complement_dict = {"A": "T", | |
"T": "A", | |
"G": "C", | |
"C": "G"} |
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.
следующие словарики я бы также подпарвил
complement_dict = {"A": "U", "U": "A", "G": "C", "C": "G"} | ||
|
||
def codons(self): | ||
return [self.sequence[i : i + 3] for i in range(0, len(self.sequence), 3)] |
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.
Может оказаться так, что последовательность не кратна 3, поэтому на такой случай стоит что-то придумать, например показывать пользователю ошибку
return [self.sequence[i : i + 3] for i in range(0, len(self.sequence), 3)] | |
if len(self.sequence) % 3 != 0: | |
raise ValueError("Sequence length is not a multiple of 3") | |
else: | |
return [self.sequence[i : i + 3] for i in range(0, len(self.sequence), 3)] |
"V": 117.15, | ||
"W": 204.23, | ||
"Y": 181.19, | ||
} |
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.
очень красивый словарик, легко читаемый
"W": 204.23, | ||
"Y": 181.19, | ||
} | ||
return sum(molecular_weights[aa] for aa in self.sequence) |
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.
Это очень хорошая работа, она структурно логичная, приятно читается, нет лишних переменных и функций, что радует. Если добавить аннотации, то к работе будет самой идеальной, но она уже на высоте
Review RHNO1