Skip to content

Review GPR183 #39

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions GPR183.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
# Importing modules
import os
from Bio import SeqIO
from Bio.SeqUtils import gc_fraction
from typing import Union
Comment on lines +2 to +5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не совсем корректно представлены импорты. Корректирую все программы с помощью 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Встроенные
  2. Сторонние
  3. Локальные

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



def fastq_filter(input_path: str = None, output_filename: str = None, *,
gc_bound: Union[tuple, int, float] = (0, 100),
length_bound: Union[tuple, int, float] = (0, 2**32),
quality_threshold: Union[int, float] = 0) -> None:
Comment on lines +9 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
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
Comment on lines +13 to +28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""
# Chech if PATH for input file is given
if input_path is None:
raise ValueError("You didn't enter any PATH to file")
Comment on lines +31 to +32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Chech if folder exist and create outout_path if not given
input_folder = input_path.rsplit('/', 1)[0]
input_name = input_path.rsplit('/', 1)[1]
is_exist = os.path.exists(f'{input_folder}/fastq_filtrator_resuls/')
if not is_exist:
os.makedirs(f'{input_folder}/fastq_filtrator_resuls/')
if output_filename is None:
output_path = f'{input_folder}/fastq_filtrator_resuls/{input_name}'
else:
output_path = f'{input_folder}/fastq_filtrator_resuls/{output_filename}.fastq'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По PEP8 очень длинная конструкция. Возможно, её стоило бы разбить на несколько

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Например можно вынести 'fastq_filtrator_resuls' в отдельную переменную

# Create dict from FASTQ
seqs = list(SeqIO.parse(input_path, "fastq"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ровно так

# Check that this dict is not empty
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!')
Comment on lines +46 to +53

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Create filtered list
filtered_fastq = []

for line in seqs:
if isinstance(gc_bound, (int, float)):
gc_check = gc_fraction(line)*100 >= gc_bound
else:
gc_check = gc_fraction(line)*100 < gc_bound[0] or gc_fraction(line)*100 > gc_bound[1]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тоже PEP8 ругается на длину.

if isinstance(length_bound, (int, float)):
len_check = len(line) >= length_bound
else:
len_check = len(line) < length_bound[0] or len(line) > length_bound[1]
quality_check = sum(line.letter_annotations['phred_quality'])/len(line.letter_annotations['phred_quality']) < quality_threshold

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Данная конструкция также абсолютно корректна, но она в себе содержит слишком много информации и маленько сложна для понимания. На мой взгляд лучше было бы ее разбить на несколько. И PEP8 тоже бы тогда не ругался)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if not (gc_check or len_check or quality_check):
filtered_fastq.append(line)
Comment on lines +67 to +68

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сложно было понять, почему проверка происходит через not. Для человека, наверное, интуитивно понятней, когда проверка обозначает под True прохождение порогов. Но для программы это не имеет значения, так что это мелочи :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Абсолютно правильный комментарий. В старом варианте кода я НЕ добавлял строку в лист, а удалял ее из словаря, поэтому синтаксически выглядело логично. К сожалению, времени было мало, и я не успел логику заменить! Но комментарий очень хороший, я когда код писал, сам об этом думал!


# Write filtered data into new .fastq file
SeqIO.write(filtered_fastq, output_path, 'fastq')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Вообще для проверок своего кода я использую flake8. Мне очень помогает.

# Custom error
class WrongSequence(ValueError):
pass
Comment on lines +74 to +75

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Круто, что сделали кастомную ошибку!



class BiologicalSequence(str):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def __init__(self, sequence):
self.sequence = sequence
Comment on lines +78 to +80

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

В данном случае можно было бы реализовать абстрактный метод следующим образом:

Suggested change
class BiologicalSequence(str):
def __init__(self, sequence):
self.sequence = sequence
class BiologicalSequence(ABC):
@abstractmethod # Указываем над каждым методом
def __init__(self, sequence):
pass

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Спасибо! Да, абстрактный класс тут подходит больше, согласен)


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
def __str__(self) -> str:
return self.sequence

def __len__(self):
return len(self.sequence)
Comment on lines +82 to +83

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
def __len__(self):
return len(self.sequence)
@abstractmethod
def __len__(self):
pass


def slice(self, start_index, stop_index):
return self.sequence[start_index:stop_index]
Comment on lines +85 to +86

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для получения элементов по индексу есть свой дандер getitem. Вот так у меня работает:

Suggested change
def slice(self, start_index, stop_index):
return self.sequence[start_index:stop_index]
@abstractmethod
def __getitem__(self, item):
pass


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Необходимо было добавить еще методы repr и str для отображения технической информации и отображения через print

def alphabet_checking(self):
if not set(self.sequence) <= set(type(self).dictionary.keys()):
raise WrongSequence('Wrong sequence')
return True
Comment on lines +88 to +91
Copy link

@EkaterinShitik EkaterinShitik Mar 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Проверка - супер разумная, но, во-первых, она должна быть определена в следующем поколении классов.
А во-вторых, она дублирует возврат 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())



class NucleicAcidSequence(BiologicalSequence):

def __init__(self, sequence):
super().__init__(sequence)
if not self.alphabet_checking():
del self.sequence
raise WrongSequence('You have entered a wrong sequence')
Comment on lines +96 to +100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Очень круто, что учли проверку при создании классов! Тут, возможно, нет необходимости удалять self.sequence. После поднятия ошибки экземпляр класса просто не создаётся.

self.gc_cont = None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Очень круто, что содержание gc внесли в отдельный атрибут. Взяла себе на заметку!


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Супер, что выполнено условие полиморфизма, поставленного в задании, но уж слишком длинная строка по PEP8. Наверное, тоже стоило бы разбить.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def gc_content(self):
gc = 0
for nucleotide in self.sequence:
if nucleotide in set('CGcg'):
gc += 1
self.gc_cont = round(gc / len(self.sequence), 3)
return self.gc_cont


class DNAsequence(NucleicAcidSequence):
dictionary = {
'A': 'T',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 't',
'g': 'c',
't': 'a',
'c': 'g',
}
Comment on lines +116 to +125

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8

Suggested change
dictionary = {
'A': 'T',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 't',
'g': 'c',
't': 'a',
'c': 'g',
}
dictionary = {
'A': 'T',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 't',
'g': 'c',
't': 'a',
'c': 'g',
}

trans_dict = {
'A': 'U',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 'u',
'g': 'c',
't': 'a',
'c': 'g',
}
Comment on lines +126 to +135

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8

Suggested change
trans_dict = {
'A': 'U',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 'u',
'g': 'c',
't': 'a',
'c': 'g',
}
trans_dict = {
'A': 'U',
'G': 'C',
'T': 'A',
'C': 'G',
'a': 'u',
'g': 'c',
't': 'a',
'c': 'g',
}

def transcribe(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Пропущена 1 строчка

Suggested change
def transcribe(self):
def transcribe(self):

return RNAsequence(''.join([self.trans_dict[i] for i in self.sequence]))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Всё супер! Возвращается не строка, а необходимый класс. И использована очень интересная конструкция для транскрипции. Но, я опять повторюсь, может стоило бы его разбить на несколько частей



class RNAsequence(NucleicAcidSequence):
dictionary = {
'A': 'U',
'G': 'C',
'U': 'A',
'C': 'G',
'a': 'u',
'g': 'c',
'u': 'a',
'c': 'g',
}
Comment on lines +141 to +150

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
}



class AminoAcidSequence(BiologicalSequence):
dictionary = {
"A": 71.03711,
"C": 103.00919,
"D": 115.02694,
"E": 129.04259,
"F": 147.06841,
"G": 57.02146,
"H": 137.05891,
"I": 113.08406,
"K": 128.09496,
"L": 113.08406,
"M": 131.04049,
"N": 114.04293,
"P": 97.05276,
"Q": 128.05858,
"R": 156.10111,
"S": 87.03203,
"T": 101.04768,
"V": 99.06841,
"W": 186.07931,
"Y": 163.06333,
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8

def __init__(self, sequence):
super().__init__(sequence)
if not self.alphabet_checking():
del self.sequence
raise WrongSequence('You have entered a wrong sequence')
Comment on lines +176 to +180

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Перед 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')


def protein_mass(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
def protein_mass(self):
def protein_mass(self) -> float:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return mass