Skip to content

Conversation

@nvaulin
Copy link
Member

@nvaulin nvaulin commented Feb 26, 2024

Review RAG2

Copy link

@MariaLuk MariaLuk left a comment

Choose a reason for hiding this comment

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

Подсмотрела интересные идеи, особенно про continue) спасибо!

from Bio import SeqIO
from Bio.SeqUtils import gc_fraction
from Bio.Seq import Seq

Copy link

Choose a reason for hiding this comment

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

PEP просит две пустых строки, после import

from Bio.SeqUtils import gc_fraction
from Bio.Seq import Seq

class BiologicalSequence(ABC):
Copy link

Choose a reason for hiding this comment

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

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

Но при этом мы обсуждали что абстрактный класс это каркас в котором должны быть только abstact методы, а определены они будут ниже в дочерних классах.

Comment on lines +43 to +48
def gc_content(self):
gc_content = (self.sequence.count('G') + self.sequence.count('C')) / len(self.sequence) if self.sequence else 0
return gc_content

class DNASequence(NucleicAcidSequence):
complement_map = {'A': 'T', 'T': 'A', 'G': 'C', 'C': 'G'}
Copy link

Choose a reason for hiding this comment

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

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

class DNASequence(NucleicAcidSequence):
complement_map = {'A': 'T', 'T': 'A', 'G': 'C', 'C': 'G'}
def transcribe(self):
return RNASequence(self.sequence.replace('T', 'U'))
Copy link

Choose a reason for hiding this comment

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

Прикольно, что сделано через replace, я так не догадалась

return all(nucleotide in self.complement_map for nucleotide in self.sequence)

def complement(self):
return ''.join(self.complement_map[nucleotide] for nucleotide 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.

По условию тут должны были возвращаться объекты классов RNASequence или DNASequence

Copy link
Member Author

Choose a reason for hiding this comment

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

Такие штуки в ревью еще здорово оформлять кодом (RNASequence), хотя в целом когда это 1-2 слова то не обязательно тратить время на такие мелочи

sequence (str): The input protein sequence in one-letter code.

Returns:
str: The converted protein sequence in three-letter code.
Copy link

Choose a reason for hiding this comment

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

Классно, что есть докстринга

Returns:
str: The converted protein sequence in three-letter code.
"""
AMINO_ACIDS = {'A': 'Ala', 'C': 'Cys', 'D': 'Asp', 'E': 'Glu', 'F': 'Phe', 'G': 'Gly', 'H': 'His', 'I': 'Ile',
Copy link

Choose a reason for hiding this comment

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

Так как это не глобальная переменная, то я бы назвала ее маленькими буквами

Comment on lines +131 to +138
if not (gc_bounds[0] <= gc_content <= gc_bounds[1]):
continue

if not (length_bounds[0] <= len(sequence) <= length_bounds[1]):
continue

if not check_quality(quality_scores, quality_threshold):
continue
Copy link

@MariaLuk MariaLuk Mar 9, 2024

Choose a reason for hiding this comment

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

Очень долго разбиралась, но поняла, что да , так должно работать. Если условие не выполнится, то все прейдет к следующей итерации! Не знала такого про continue, либо знала, но забыла, очень круто, хочу попробовать пользоваться. Здорово, что каждая запись последовательно отрабатывается на три фильтра

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


filtered_seqs[record.id] = (sequence, quality_scores)

if output_filename:
Copy link

@MariaLuk MariaLuk Mar 9, 2024

Choose a reason for hiding this comment

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

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

И еще такой момент, получается в запись идет только последовательность и качество, и теряется исходный формат записи fastq файла

if not (length_bounds[0] <= len(sequence) <= length_bounds[1]):
continue

if not check_quality(quality_scores, quality_threshold):
Copy link

Choose a reason for hiding this comment

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

Сначала думала, что функция check_quality потерялась, потом нашла ее
Мне кажется, это не очень гуманным к проверяющему оставлять почти 500 строк никак не используемого кода в формате дз четким тз))
Там, конечно ,есть классные и интересные функции, и я бы даже почитала ВСЕ) но если я буду выполнять задание "провести код ревью", то тут получится код ревью без границ просто. Опять же, check_quality где-то там потерялась)

return '-'.join(three_letter_code)


def filter_fastq(input_path: str, output_filename: str = None, gc_bounds: tuple = (0, 100), length_bounds: tuple = (0, 2 ** 32),
Copy link

Choose a reason for hiding this comment

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

Нас просили переписать, заменить функцию фильтратор. Когда их две, я могу потеряться)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants