Review EVI5L by nvaulin · Pull Request #35 · Python-BI-2023/Peer_review

nvaulin · 2024-02-26T17:57:35Z

Review EVI5L

ailiskab-hub

Ну главное, что вы сделали задание)

ailiskab-hub · 2024-03-07T14:23:08Z

+        -output (dict[str,str]) - filtered dictionary, which consists of entities that fulfill entered parameters.
+    """
+
+    def average_quality(record):


Лучше отпределять функцию вне функции

ailiskab-hub · 2024-03-07T14:25:55Z

+    def __init__(self, sequence):
+        self.sequence = sequence
+
+    @abstractmethod


По моему тут чего то не хватает

ailiskab-hub · 2024-03-07T14:27:50Z

+    sequence_type: str = None  # Это будет переопределено в подклассах
+
+    def __init__(self, sequence):
+        self.sequence = sequence


Тут необходимо добавить структуру с super(), чтобы наследование проходило корректно

ailiskab-hub · 2024-03-07T14:28:47Z

+        return ''.join(COMPLEMENT_MAP[base] for base in self.sequence)
+
+    def check_alphabet(self):
+        valid_bases = set('ATGCU') if self.sequence_type == 'DNA' else set('AUGC')


Можно ввести переменные, чтобы не создавать множество кадый раз

Првильно

ailiskab-hub · 2024-03-07T14:30:22Z

+
+
+class DNASequence(NucleicAcidSequence):
+    sequence_type = 'DNA'


Если sequence_type это атрибут модели, то нужно было использовать self.sequence_type
И снова не хватает super()

ailiskab-hub · 2024-03-07T14:30:50Z

+        return (self.complement()).replace('T', 'U')
+
+
+class RNASequence(NucleicAcidSequence):


Аналогично прошлому случаю

ailiskab-hub · 2024-03-07T14:31:37Z

+        return self.sequence[index]
+
+    def __str__(self):
+        return self.sequence


Очень много дублирующегося кода в ДНК и РНК, почему бы не реализовать это в родительском классе?

ailiskab-hub · 2024-03-07T14:33:03Z

+                              'E': 147.131, 'Q': 146.146, 'G': 75.067, 'H': 155.156, 'I': 131.175,
+                              'L': 131.175, 'K': 146.189, 'M': 149.208, 'F': 165.192, 'P': 115.132,
+                              'S': 105.093, 'T': 119.119, 'W': 204.228, 'Y': 181.191, 'V': 117.148}
+        return sum(MOLECULAR_MASS_MAP[aa] for aa in self.sequence)


Тут тоже много дублирующегося кода, некоторые из методов можно было реализовать в BiologicalSequence, чтобы избегать повторения

stegodasha

Вообще в целом ты большой/ая молодец))

stegodasha · 2024-03-09T12:29:38Z

+            if len(seq) >= length_bounds[0] and len(seq) <= length_bounds[1] \
+                    and average_quality(seq) >= quality_threshold \
+                    and GC(seq.seq) >= gc_bounds[0] and GC(seq.seq) <= gc_bounds[1]:


Я бы постаралась вычислить GC состав один раз:

Suggested change

if len(seq) >= length_bounds[0] and len(seq) <= length_bounds[1] \

and average_quality(seq) >= quality_threshold \

and GC(seq.seq) >= gc_bounds[0] and GC(seq.seq) <= gc_bounds[1]:

gc_content = GC(seq.seq)

if len(seq) >= length_bounds[0] and len(seq) <= length_bounds[1] \

and average_quality(seq) >= quality_threshold \

and gc_bounds[0] <= gc_content <= gc_bounds[1]:

stegodasha · 2024-03-09T12:33:52Z

+        COMPLEMENT_MAP = {'A': 'T', 'T': 'A', 'C': 'G', 'G': 'C', 'U': 'A'} if self.sequence_type == 'DNA' else {
+            'A': 'U', 'U': 'A', 'C': 'G', 'G': 'C'}


Если COMPLEMENT_MAP предполагается быть константой, лучше выделить его как атрибут класса или объявить его вне метода.

Ага, и когда он будет классовым атрибутом - то его уже не надо делать капсом

stegodasha · 2024-03-09T12:39:13Z

+    def transcribe(self):
+        return (self.complement()).replace('T', 'U')


Я бы предложила использовать super() для вызова метода complement из родительского класса, чтобы убедиться, что будет использована правильная версия метода: return super().complement().replace('T', 'U')

stegodasha · 2024-03-09T12:41:27Z

+    def __len__(self):
+        return len(self.sequence)
+
+    def __getitem__(self, index):
+        return self.sequence[index]
+
+    def __str__(self):
+        return self.sequence


Согласна с Алисой, я бы вынесла эти методы в родительский класс, чтобы их не дублировать

stegodasha · 2024-03-09T15:59:35Z

+        MOLECULAR_MASS_MAP = {'A': 89.094, 'R': 174.203, 'N': 132.119, 'D': 133.104, 'C': 121.154,
+                              'E': 147.131, 'Q': 146.146, 'G': 75.067, 'H': 155.156, 'I': 131.175,
+                              'L': 131.175, 'K': 146.189, 'M': 149.208, 'F': 165.192, 'P': 115.132,
+                              'S': 105.093, 'T': 119.119, 'W': 204.228, 'Y': 181.191, 'V': 117.148}


Я бы вынесла MOLECULAR_MASS_MAP отдельно как атрибут класса.

Add EVI5L.py

951a3a6

ailiskab-hub reviewed Mar 7, 2024

View reviewed changes

stegodasha reviewed Mar 9, 2024

View reviewed changes



		class DNASequence(NucleicAcidSequence):
		sequence_type = 'DNA'

		return (self.complement()).replace('T', 'U')


		class RNASequence(NucleicAcidSequence):

		COMPLEMENT_MAP = {'A': 'T', 'T': 'A', 'C': 'G', 'G': 'C', 'U': 'A'} if self.sequence_type == 'DNA' else {
		'A': 'U', 'U': 'A', 'C': 'G', 'G': 'C'}

		def transcribe(self):
		return (self.complement()).replace('T', 'U')

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

ailiskab-hub 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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants