Skip to content

Review RNF34 #41

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
134 changes: 134 additions & 0 deletions RNF34.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
from abc import ABC, abstractmethod


class BiologicalSequence(ABC):

Choose a reason for hiding this comment

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

Прекрасная работа с абстрактным классом. Все необходимые абстрактные методы учтены!

@abstractmethod
def __init__(self, sequence):
pass

@abstractmethod
def __len__(self):
pass

@abstractmethod
def __getitem__(self, slc):
pass

@abstractmethod
def __str__(self):
pass

@abstractmethod
def __repr__(self):
pass

@abstractmethod
def is_valid_alphabet(self):
pass


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

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

def __getitem__(self, slc):
return self.sequence[slc]

def __str__(self):
return str(self.sequence)

Choose a reason for hiding this comment

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

Хорошо, что перестраховались и точно определили строковую переменную) Но, тут это не обязательно, так как self.sequence - уже строка.

Suggested change
return str(self.sequence)
return self.sequence


def __repr__(self):
return self.sequence

Choose a reason for hiding this comment

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

В методе repr мы должны скорее передавать техническую информацию о классе. Я тоже в своей дз передала self.sequence, но при ревью подумала, что правильнее было бы использовать строку c указанием класса.

Suggested change
return self.sequence
return f'NucleicAcid("{self.sequence}")'

Copy link
Member Author

Choose a reason for hiding this comment

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

Верно подмечено!


def is_valid_alphabet(self):
alphabet = type(self).ALPHABET

Choose a reason for hiding this comment

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

Класс! Учтено условие задания по полиморфизма классов NucleicAcid, DNASequence и RNASequence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Я бы еще добавил что поскольку алфавит больше не внешняя константа, а классновый атрибут
самый обычный классовый атрибут

то его больше не надо писать в капсе

if set(self.sequence).issubset(alphabet):
return True
else:
return False
Comment on lines +48 to +51

Choose a reason for hiding this comment

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

Очень разумная проверка, однако в данном случае нет необходимости возвращать явным образом True и False. Вот так будет отлично работать:

Suggested change
if set(self.sequence).issubset(alphabet):
return True
else:
return False
return set(self.sequence).issubset(alphabet)


def complement(self):
if type(self) == NucleicAcid:
raise NotImplementedError("Cannot complement NucleicAcid instance")
Comment on lines +54 to +55

Choose a reason for hiding this comment

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

Отлично, что делаете проверки на тип подаваемой последовательности! Однако, в рамках данной проверки не учитывается различие ДНК и РНК последовательностей. На мой взгляд, было бы корректней их разделить на каждый класс.

Также проверки по типу лучше делать через конструкцию is

Suggested change
if type(self) == NucleicAcid:
raise NotImplementedError("Cannot complement NucleicAcid instance")
if type(self) is NucleicAcid:
raise NotImplementedError("Cannot complement NucleicAcid instance")


map_dict = type(self).MAP
comp_seq = "".join([map_dict[base] for base in self.sequence])

Choose a reason for hiding this comment

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

Классная конструкция! Я взяла себе на заметку)


return type(self)(comp_seq)

Choose a reason for hiding this comment

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

Супер! Возвращаются не просто строковые переменные, но именно экземпляры определённого класса.


def gc_content(self):
if type(self) == NucleicAcid:
raise NotImplementedError("Cannot gc_content NucleicAcid instance")
Comment on lines +63 to +64

Choose a reason for hiding this comment

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

Suggested change
if type(self) == NucleicAcid:
raise NotImplementedError("Cannot gc_content NucleicAcid instance")
if type(self) is NucleicAcid:
raise NotImplementedError("Cannot gc_content NucleicAcid instance")

gc_count = sum([1 for base in self.sequence if base in ["C", "G"]])

Choose a reason for hiding this comment

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

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

gc_content = (gc_count / len(self)) * 100

return gc_content


class DNASequence(NucleicAcid):
ALPHABET = set("ATGC")
MAP = {"A": "T", "T": "A", "C": "G", "G": "C"}
Comment on lines +72 to +73

Choose a reason for hiding this comment

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

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

Suggested change
ALPHABET = set("ATGC")
MAP = {"A": "T", "T": "A", "C": "G", "G": "C"}
alphabet = set("ATGC")
map = {"A": "T", "T": "A", "C": "G", "G": "C"}

Copy link
Member Author

Choose a reason for hiding this comment

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

а о, супер, да


def transcribe(self):
transcribed = self.sequence.replace("T", "U")

return RNASequence(transcribed)


class RNASequence(NucleicAcid):

Choose a reason for hiding this comment

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

Классы DNASequence и RNASequence содержат все необходимые методы, определённые по условию задачи. Однако, в них не определена проверка на соответствие последовательности классу. Грубо говоря, конструкция RNASequence('ATGC') отлично работает, но это биологически не корректно.

Ранее мы определили метод is_valid_alphabet(), который отлично бы использовался в данных целях. Для того, чтобы его реализовать мы можем переопределить init нового класса. Например, я сделала так:

Suggested change
class RNASequence(NucleicAcid):
class RNASequence(NucleicAcid):
def __init__(self, seq):
super().__init__(seq)
if not (super().is_valid_alphabet()):
raise ValueError('The sequence does not correspond to RNA')

ALPHABET = set("AUGC")
MAP = {"A": "U", "U": "A", "C": "G", "G": "C"}
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.

Suggested change
ALPHABET = set("AUGC")
MAP = {"A": "U", "U": "A", "C": "G", "G": "C"}
alphabet = set("AUGC")
map = {"A": "U", "U": "A", "C": "G", "G": "C"}



class AminoAcidSequence(BiologicalSequence):

Choose a reason for hiding this comment

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

Тоже было бы неплохо сделать проверку на соответствие АМК последовательности

ALPHABET = set("ACDEFGHIKLMNPQRSTVWY")

Choose a reason for hiding this comment

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

Suggested change
ALPHABET = set("ACDEFGHIKLMNPQRSTVWY")
alphabet = set("ACDEFGHIKLMNPQRSTVWY")


def __init__(self, sequence):
self.sequence = sequence

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

def __getitem__(self, slc):
return self.sequence[slc]

def __str__(self):
return str(self.sequence)
Comment on lines +98 to +99

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):
return str(self.sequence)
def __str__(self):
return self.sequence


def __repr__(self):
return self.sequence
Comment on lines +101 to +102

Choose a reason for hiding this comment

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

Suggested change
def __repr__(self):
return self.sequence
def __repr__(self):
return f'AminoAcidSequence("{self.sequence}")'


def is_valid_alphabet(self):
alphabet = type(self).ALPHABET
if set(self.sequence).issubset(alphabet):
return True
else:
return False
Comment on lines +106 to +109

Choose a reason for hiding this comment

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

Suggested change
if set(self.sequence).issubset(alphabet):
return True
else:
return False
return set(self.sequence).issubset(alphabet):


amino_acid_frequency = {}

def calculate_aa_freq(self):
"""
Calculates the frequency of each amino acid in a protein sequence or sequences.

:param sequences: protein sequence or sequences
:type sequences: str or list of str
:return: dictionary with the frequency of each amino acid
:rtype: dict
"""
Comment on lines +114 to +121

Choose a reason for hiding this comment

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

Можно было бы маленько подправить докстринг при переносе функции в новую программу :)
И тут же добавлю, что во всем коде совсем нет аннотаций( Кажется, с ними правда приятней смотреть на код.


# Creating a dictionary with aminoacid frequencies:
amino_acid_frequency = {}

for amino_acid in self.sequence:
# If the aminoacid has been already in:
if amino_acid in amino_acid_frequency:
amino_acid_frequency[amino_acid] += 1
# If the aminoacid hasn't been already in:
else:
amino_acid_frequency[amino_acid] = 1

return amino_acid_frequency