Review RNF34 #41
Review RNF34 #41
Conversation
There was a problem hiding this comment.
Очень хорошая работа! Все условия наследования и полиморфизма учтены. Код написан в соответствии с PEP8. Я бы выделила следующие основные замечания:
- Не реализована функция is_valid_alphabet(), с помощью которой можно было бы проверять соответствие последовательности при создании экземпляров класса
- В коде нет аннотаций
Ну и конечно, не выполнено задание по парсеру fastq с использованием BioPython. Мне кажется, что вы бы легко с ним справились)
| from abc import ABC, abstractmethod | ||
|
|
||
|
|
||
| class BiologicalSequence(ABC): |
There was a problem hiding this comment.
Прекрасная работа с абстрактным классом. Все необходимые абстрактные методы учтены!
| return self.sequence[slc] | ||
|
|
||
| def __str__(self): | ||
| return str(self.sequence) |
There was a problem hiding this comment.
Хорошо, что перестраховались и точно определили строковую переменную) Но, тут это не обязательно, так как self.sequence - уже строка.
| return str(self.sequence) | |
| return self.sequence |
| return str(self.sequence) | ||
|
|
||
| def __repr__(self): | ||
| return self.sequence |
There was a problem hiding this comment.
В методе repr мы должны скорее передавать техническую информацию о классе. Я тоже в своей дз передала self.sequence, но при ревью подумала, что правильнее было бы использовать строку c указанием класса.
| return self.sequence | |
| return f'NucleicAcid("{self.sequence}")' |
| return self.sequence | ||
|
|
||
| def is_valid_alphabet(self): | ||
| alphabet = type(self).ALPHABET |
There was a problem hiding this comment.
Класс! Учтено условие задания по полиморфизма классов NucleicAcid, DNASequence и RNASequence.
There was a problem hiding this comment.
Я бы еще добавил что поскольку алфавит больше не внешняя константа, а классновый атрибут
самый обычный классовый атрибут
то его больше не надо писать в капсе
| if set(self.sequence).issubset(alphabet): | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
Очень разумная проверка, однако в данном случае нет необходимости возвращать явным образом True и False. Вот так будет отлично работать:
| if set(self.sequence).issubset(alphabet): | |
| return True | |
| else: | |
| return False | |
| return set(self.sequence).issubset(alphabet) |
|
|
||
|
|
||
| class AminoAcidSequence(BiologicalSequence): | ||
| ALPHABET = set("ACDEFGHIKLMNPQRSTVWY") |
There was a problem hiding this comment.
| ALPHABET = set("ACDEFGHIKLMNPQRSTVWY") | |
| alphabet = set("ACDEFGHIKLMNPQRSTVWY") |
| def __str__(self): | ||
| return str(self.sequence) |
There was a problem hiding this comment.
| def __str__(self): | |
| return str(self.sequence) | |
| def __str__(self): | |
| return self.sequence |
| def __repr__(self): | ||
| return self.sequence |
There was a problem hiding this comment.
| def __repr__(self): | |
| return self.sequence | |
| def __repr__(self): | |
| return f'AminoAcidSequence("{self.sequence}")' |
| if set(self.sequence).issubset(alphabet): | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
| if set(self.sequence).issubset(alphabet): | |
| return True | |
| else: | |
| return False | |
| return set(self.sequence).issubset(alphabet): |
| """ | ||
| 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 | ||
| """ |
There was a problem hiding this comment.
Можно было бы маленько подправить докстринг при переносе функции в новую программу :)
И тут же добавлю, что во всем коде совсем нет аннотаций( Кажется, с ними правда приятней смотреть на код.
Review RNF34