Review GPR183 by nvaulin · Pull Request #39 · Python-BI-2023/Peer_review

nvaulin · 2024-02-26T17:57:46Z

Review GPR183

EkaterinShitik

Учитывая объем задания, работа выполнена очень хорошо!

В парсере fastq реализован весь функционал и использованы все необходимые модули из BioPython
В задании по классам выполнены все условия полиморфизма и наследования

Из моментов для доработки я бы подстветила несколько моментов.

BiologicalSequence не реализован, как абстрактный класс. Соответственно, он не опеределяет свод правил для всех дочерних классов. Плюсом он подтягивает методы строки, которые нам могут даже помешать.
Очень много мелких моментов PEP8. Автоматизированные проверки спасают!
Нет аннотаций типов.

В любом случае, вы молодец!

EkaterinShitik · 2024-03-04T14:50:06Z

GPR183.py

+import os
+from Bio import SeqIO
+from Bio.SeqUtils import gc_fraction
+from typing import Union


Не совсем корректно представлены импорты. Корректирую все программы с помощью isort)

Suggested change

import os

from Bio import SeqIO

from Bio.SeqUtils import gc_fraction

from typing import Union

import os

from typing import Union

from Bio import SeqIO

from Bio.SeqUtils import gc_fraction

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

Встроенные

Сторонние

Локальные

Между ними по строке, так что Катя (в лице isort'a) и isort (в лице Кати) правы

EkaterinShitik · 2024-03-04T15:19:59Z

GPR183.py

+    This function work with FASTQ files and filters them by
+    GC content, length and Q-score.
+
+    Arguments (positional):
+    - input_path (str): full path to the file that you want to work with
+    - output_filename (str): enter just a name of the file, don't add extention
+
+    Arguments (keyword):
+    - gc_bound (tuple, int, float): tuple of required range of GC percentage (inclusive),
+    num or float if only higher border of the range is needed (exclusive).
+    - length_bound (tuple, int, float): tuple of required range of sequences length (inclusive),
+    num or float if only higher border of the range is needed (exclusive).
+    - quality_threshold (int): int of lowest level of Q-score (inclusive).
+
+    Output:
+    - list of BioSeq records. Write file to .fastq


Классный и понятный докстринг!

EkaterinShitik · 2024-03-04T15:21:06Z

GPR183.py

+    if input_path is None:
+        raise ValueError("You didn't enter any PATH to file")


Очень полезная ошибка) Облегчает работу с программой!

EkaterinShitik · 2024-03-04T15:27:06Z

GPR183.py

+    else:
+        output_path = f'{input_folder}/fastq_filtrator_resuls/{output_filename}.fastq'
+    # Create dict from FASTQ
+    seqs = list(SeqIO.parse(input_path, "fastq"))


Всё указано супер корректно, но не совсем понятно, зачем нам полученный результат переводить в лист?
При использовании парсера из BioPython, мы уже получаем коллекцию из SeqRecord, по которой можем итерироваться. У меня вот так работает :)

Suggested change

seqs = list(SeqIO.parse(input_path, "fastq"))

seqs = SeqIO.parse(input_path, "fastq")

Ровно так

EkaterinShitik · 2024-03-04T15:29:24Z

GPR183.py

+    if len(seqs) <= 0:
+        raise ValueError('There are no fastq sequences')
+    # Check if all given argumets have relevant type
+    gc_bound_type = isinstance(gc_bound, (tuple, int, float))
+    length_bound_type = isinstance(length_bound, (tuple, int, float))
+    quality_thr_type = isinstance(quality_threshold, (int, float))
+    if not (gc_bound_type and length_bound_type and quality_thr_type):
+        raise ValueError('Your arguments are not suitable!')


Тоже супер классные и полезные ошибки! Взяла себе на заметку!

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

EkaterinShitik · 2024-03-04T16:15:13Z

GPR183.py

+    dictionary = {
+        'A': 'U',
+        'G': 'C',
+        'U': 'A',
+        'C': 'G',
+        'a': 'u',
+        'g': 'c',
+        'u': 'a',
+        'c': 'g',
+}


PEP8

Suggested change

dictionary = {

'A': 'U',

'G': 'C',

'U': 'A',

'C': 'G',

'a': 'u',

'g': 'c',

'u': 'a',

'c': 'g',

}

dictionary = {

'A': 'U',

'G': 'C',

'U': 'A',

'C': 'G',

'a': 'u',

'g': 'c',

'u': 'a',

'c': 'g',

}

EkaterinShitik · 2024-03-04T16:25:58Z

GPR183.py

+    def __len__(self):
+        return len(self.sequence)


Вернусь к теме про абстрактный класс. BiologicalSequence должен был быть просто шаблоном, определяющим правила создания всех последующих классов. В данном шаблоне мы не должны прописывать никакие функции, так как они не выполняются, а только являются сводом правил. Данный код необходимо было перенести в дочерние классы. В данном случае, это NucleicAcidSequence и AminoAcidSequence

Suggested change

def __len__(self):

return len(self.sequence)

@abstractmethod

def __len__(self):

pass

EkaterinShitik · 2024-03-04T16:29:40Z

GPR183.py

+    def alphabet_checking(self):
+        if not set(self.sequence) <= set(type(self).dictionary.keys()):
+            raise WrongSequence('Wrong sequence')
+        return True


Проверка - супер разумная, но, во-первых, она должна быть определена в следующем поколении классов.
А во-вторых, она дублирует возврат True. Само условие уже подразумевает возвращение True или False, поэтому лучше бы сделать так

Suggested change

def alphabet_checking(self):

if not set(self.sequence) <= set(type(self).dictionary.keys()):

raise WrongSequence('Wrong sequence')

return True

def alphabet_checking(self):

return set(self.sequence) <= set(type(self).dictionary.keys())

EkaterinShitik · 2024-03-04T16:32:46Z

GPR183.py

+    def __init__(self, sequence):
+        super().__init__(sequence)
+        if not self.alphabet_checking():
+            del self.sequence
+            raise WrongSequence('You have entered a wrong sequence')


Перед init должна быть одна строчка

Suggested change

def __init__(self, sequence):

super().__init__(sequence)

if not self.alphabet_checking():

del self.sequence

raise WrongSequence('You have entered a wrong sequence')

def __init__(self, sequence):

super().__init__(sequence)

if not self.alphabet_checking():

raise WrongSequence('You have entered a wrong sequence')

EkaterinShitik · 2024-03-04T16:49:35Z

GPR183.py

+            del self.sequence
+            raise WrongSequence('You have entered a wrong sequence')
+
+    def protein_mass(self):


C функцией всё отлично, но тут добавлю комментарий по всему заданию по классам. Нигде не приведена аннотация. Кажется, с ней просматривать код намного приятней)

Suggested change

def protein_mass(self):

def protein_mass(self) -> float:

sme229 · 2024-03-05T17:55:29Z

GPR183.py

+                 gc_bound: Union[tuple, int, float] = (0, 100),
+                 length_bound: Union[tuple, int, float] = (0, 2**32),
+                 quality_threshold: Union[int, float] = 0) -> None:


Интересное решение с Union, взяла себе на заметку

sme229 · 2024-03-05T18:08:42Z

GPR183.py

+            raise WrongSequence('You have entered a wrong sequence')
+
+    def protein_mass(self):
+        mass = sum(self.dictionary.get(aa) for aa in self.sequence)


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

Suggested change

mass = sum(self.dictionary.get(aa) for aa in self.sequence)

list_input_seq = list(seq)

water_mw = 18

for aa in list_input_seq:

total_mw = sum(aa_weight_dict[a] for a in list_input_seq)

mw_water_removed = (total_mw - (water_mw * (len(list_input_seq)-1)))

return mw_water_removed

Хе-хе, вот она разница между биоинформатиком и программистом!

sme229 · 2024-03-05T18:21:17Z

GPR183.py

+    pass
+
+
+class BiologicalSequence(str):


По заданию, BiologicalSequence должен был быть абстрактным классом, то есть никакого функционала тут быть не должно. Абстрактные классы создают только шаблон структуры.

sme229 · 2024-03-05T18:23:23Z

GPR183.py

+        self.gc_cont = None
+
+    def complement(self):
+        return type(self)(''.join([type(self).dictionary[i] for i in self.sequence]))


Отличное решение!

sme229 · 2024-03-05T20:46:05Z

GPR183.py

+class BiologicalSequence(str):
+    def __init__(self, sequence):
+        self.sequence = sequence
+


еще по заданию нужно было добавить вот это:

Suggested change

def __str__(self) -> str:

return self.sequence

Zoea1

Очень классная работа, мне особо и сказать нечего (тем более, что много чего уже было сказано до меня). Ты молодец!

Zoea1 · 2024-03-10T09:48:05Z

GPR183.py

+    pass
+
+
+class BiologicalSequence(str):


По заданию это должен был быть абстрактный класс.

Zoea1 · 2024-03-10T09:50:18Z

GPR183.py

+        "V": 99.06841,
+        "W": 186.07931,
+        "Y": 163.06333,
+    }


nvaulin · 2024-03-17T20:22:52Z

Очень классная работа, мне особо и сказать нечего (тем более, что много чего уже было сказано до меня). Ты молодец!

Ну тут все таки важно чтобы вы учились комментировать код, поэтому нет ничего плохого в том чтобы повторять чьи-то комментарии если вам тоже они пришли в голову

Add GPR183.py

1ef426e

EkaterinShitik reviewed Mar 4, 2024

View reviewed changes

sme229 reviewed Mar 5, 2024

View reviewed changes

sme229 reviewed Mar 8, 2024

View reviewed changes

Zoea1 reviewed Mar 10, 2024

View reviewed changes

		if input_path is None:
		raise ValueError("You didn't enter any PATH to file")

	seqs = list(SeqIO.parse(input_path, "fastq"))
	seqs = SeqIO.parse(input_path, "fastq")

-        mass = sum(self.dictionary.get(aa) for aa in self.sequence)
+    list_input_seq = list(seq)
+    water_mw = 18
+    for aa in list_input_seq:
+        total_mw = sum(aa_weight_dict[a] for a in list_input_seq)
+        mw_water_removed = (total_mw - (water_mw * (len(list_input_seq)-1)))
+    return mw_water_removed

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

EkaterinShitik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EkaterinShitik Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zoea1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvaulin commented Mar 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EkaterinShitik Mar 4, 2024 •

edited

Loading