Review CNTN6 by nvaulin · Pull Request #21 · Python-BI-2023/Peer_review

nvaulin · 2024-02-26T17:54:39Z

Review CNTN6

CaptnClementine

Чуть-чуть запутались в сложной теме с асбтрактным классом. Все остальное сделано очень хорошо, и получилось возвращать нужного типа при транскрипции, и освоили super (что мне показалось очень сложным!). Тут хочется добавить про вот такую фишку: можно в классе NucleicAcidSequence задать переменные, которые у классов уже должны быть. Но не задавать их значения. вот так:
def init(self, sequence: str, alphabet: dict = None, complement_alphabet: dict = None):
self.sequence = None
self.alphabet = None
self.complement_alphabet = None

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

Вот такое прописываем в родительском нуклеотидном классе
def setter(self, sequence: str, alphabet: dict, complement_alphabet: dict = None):
self.sequence = sequence
self.alphabet = alphabet
self.complement_alphabet = complement_alphabet

А в дочернем, например РНК - вызываем его уже с алфавитом, заданном в РНК:
def init(self, sequence: str):
super().setter(sequence, ALPHABET, REV_TRANSCRTPTION_COMPLEMENT)

Так получится хранить каждый нужный алфавит в своем классе.

Хорошая работа!

CaptnClementine · 2024-03-03T14:05:57Z

CNTN6.py

@@ -0,0 +1,88 @@
+class BiologicalSequence:
+    def __init__(self, sequence):
+        self.sequence = sequence


Суть абстрактного класса - не держать в себе готовые атрибуты и методы, а только говорить какие атрибуты и/или методы должны обязательно быть у его наследников. Здесь выходит так, что у наследуемых классов с нуклеотидными последовательностями будет алфавит белков по умолчанию :(

CaptnClementine · 2024-03-03T14:07:20Z

CNTN6.py

@@ -0,0 +1,88 @@
+class BiologicalSequence:
+    def __init__(self, sequence):


В целом, как говорили на паре, идеальный абстрактный класс init не содержит (чтобы нельзя было создавать его экзмепляры). Но тут конечно логично, что у каждого наследника должна быть последовательность, но лучше бы переписать это все в наследниках.

CaptnClementine · 2024-03-03T14:08:52Z

CNTN6.py

+    def __repr__(self):
+        return f'{self.__class__.__name__}({self.sequence!r})'


Класс, даже repr не забыт! И все методы строки есть! Супер!

CaptnClementine · 2024-03-03T14:10:44Z

CNTN6.py

+                               'S': 'Ser', 'T': 'Thr', 'W': 'Trp', 'Y': 'Tyr', 'V': 'Val'}
+
+    def __init__(self, sequence):
+        super().__init__(sequence)


Suggested change

super().__init__(sequence)

super().__init__(sequence)

self.ALPHABET = {'A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'Y'}

кажется тут смотрится логично :)

CaptnClementine · 2024-03-03T14:15:17Z

CNTN6.py

+        for char in self.sequence:
+            if char not in self.ALPHABET:
+                return False
+        return True


Suggested change

for char in self.sequence:

if char not in self.ALPHABET:

return False

return True

unique_chars = set(self.sequence)

if not (unique_chars <= self.ALPHABET):

return False

return True

Можно избежать лишних циелов используя set :) Это не обязательно, но кажется будет работать чуть быстрее. Особенно если алфивиты тоже сразу сделаны сетами - удобная вещь!)

CaptnClementine · 2024-03-03T14:24:41Z

CNTN6.py

+    def gc_content(self):
+        return (self.sequence.count('G') + self.sequence.count('C'))/len(self.sequence)


О, супер! Правда очень логично держать эту функцию тут - мы ведь ее можем и для РНК, и для ДНК использовать!

CaptnClementine · 2024-03-03T14:26:25Z

CNTN6.py

+
+
+class RNASequence(NucleicAcidSequence):
+    REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}


о! Класс!

алфавит тоже сюда можн перенести)

Suggested change

REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}

REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}

ALPHABET = {'A','U','G','C'}

CaptnClementine · 2024-03-03T14:28:46Z

CNTN6.py

+
+    def __init__(self, sequence):
+        super().__init__(sequence)
+        self.ALPHABET = {'A','U','G','C'}


Suggested change

self.ALPHABET = {'A','U','G','C'}

Нет смысл держать его тут, он же относится ко всему классу. Зачем на его переобъявлять для каждого экземпляра?

CaptnClementine · 2024-03-03T14:29:27Z

CNTN6.py

+    def reverse_transribe(self):
+        return DnaSequence(self.complement(self.REV_TRANSCRTPTION_COMPLEMENT))


Супер! И дополнительная функция, и еще и может возвращать другой класс!!!

CaptnClementine · 2024-03-03T14:30:00Z

CNTN6.py

+    TRANSCRTPTION_COMPLEMENT = {'A': 'U', 'T': 'A', 'C': 'G', 'G': 'C'}
+    def __init__(self, sequence):
+        super().__init__(sequence)
+        self.ALPHABET = {'A', 'T', 'G', 'C'}


Suggested change

TRANSCRTPTION_COMPLEMENT = {'A': 'U', 'T': 'A', 'C': 'G', 'G': 'C'}

def __init__(self, sequence):

super().__init__(sequence)

self.ALPHABET = {'A', 'T', 'G', 'C'}

TRANSCRTPTION_COMPLEMENT = {'A': 'U', 'T': 'A', 'C': 'G', 'G': 'C'}

ALPHABET = {'A', 'T', 'G', 'C'}

def __init__(self, sequence):

super().__init__(sequence)

yvolko · 2024-03-07T19:05:23Z

CNTN6.py

+class BiologicalSequence:
+    def __init__(self, sequence):
+        self.sequence = sequence
+        self.ALPHABET = {'A', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'K', 'L', 'M', 'N', 'P', 'Q', 'R', 'S', 'T', 'V', 'W', 'Y'}


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

yvolko · 2024-03-07T19:06:48Z

CNTN6.py

@@ -0,0 +1,88 @@
+class BiologicalSequence:
+    def __init__(self, sequence):


не должно быть инита в абстрактном классе

yvolko · 2024-03-07T19:09:07Z

CNTN6.py

+class NucleicAcidSequence(BiologicalSequence):
+    def complement(self, COMPLEMENT_NUCLEOTIDES=None):
+        if COMPLEMENT_NUCLEOTIDES is None:
+            COMPLEMENT_NUCLEOTIDES = {'A': 'T', 'T': 'A', 'C': 'G', 'G': 'C'}


Наверное эта часть должна быть в ДНК и РНК отдельно, так как это позволит сразу создать их с верным алфавитом

yvolko · 2024-03-07T19:10:43Z

CNTN6.py

+        self.ALPHABET = {'A','U','G','C'}
+
+    def reverse_transribe(self):
+        return DnaSequence(self.complement(self.REV_TRANSCRTPTION_COMPLEMENT))


верно, что возвращает объект правильного класса

yvolko · 2024-03-07T19:17:10Z

CNTN6.py

+    REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}
+
+    def __init__(self, sequence):
+        super().__init__(sequence)


если я не ошибаюсь, получается что все super().__init__ отсылаются к биологической последовательности, но было бы лучше если бы они были в самих классах ДНК, РНК и белоков

nvaulin · 2024-03-17T22:11:54Z

Чуть-чуть запутались в сложной теме с асбтрактным классом. Все остальное сделано очень хорошо, и получилось возвращать нужного типа при транскрипции, и освоили super (что мне показалось очень сложным!). Тут хочется добавить про вот такую фишку: можно в классе NucleicAcidSequence задать переменные, которые у классов уже должны быть. Но не задавать их значения. вот так: def init(self, sequence: str, alphabet: dict = None, complement_alphabet: dict = None): self.sequence = None self.alphabet = None self.complement_alphabet = None

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

Вот такое прописываем в родительском нуклеотидном классе def setter(self, sequence: str, alphabet: dict, complement_alphabet: dict = None): self.sequence = sequence self.alphabet = alphabet self.complement_alphabet = complement_alphabet

А в дочернем, например РНК - вызываем его уже с алфавитом, заданном в РНК: def init(self, sequence: str): super().setter(sequence, ALPHABET, REV_TRANSCRTPTION_COMPLEMENT)

Так получится хранить каждый нужный алфавит в своем классе.

Хорошая работа!

Тут прям очень большие куски кода в тексте. Это очень здорово! Но в таком случае их точно надо оформлять штрихами (клавиша ё) чтобы было как код

Add CNTN6.py

b86640d

CaptnClementine reviewed Mar 3, 2024

View reviewed changes

yvolko reviewed Mar 7, 2024

View reviewed changes

		@@ -0,0 +1,88 @@
		class BiologicalSequence:
		def __init__(self, sequence):

		def __repr__(self):
		return f'{self.__class__.__name__}({self.sequence!r})'

		def gc_content(self):
		return (self.sequence.count('G') + self.sequence.count('C'))/len(self.sequence)



		class RNASequence(NucleicAcidSequence):
		REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}

	REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}
	REV_TRANSCRTPTION_COMPLEMENT = {'A': 'T', 'U': 'A', 'C': 'G', 'G': 'C'}
	ALPHABET = {'A','U','G','C'}

		def reverse_transribe(self):
		return DnaSequence(self.complement(self.REV_TRANSCRTPTION_COMPLEMENT))

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

CaptnClementine 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!

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!

nvaulin commented Mar 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants