Review ITGA7 by nvaulin · Pull Request #20 · Python-BI-2023/Peer_review

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

Review ITGA7

NSapozhnikov

Код классный, чистый и красивый. Мне все очень понравилось, за исключением пары мест, где я решил все же придраться :)

NSapozhnikov · 2024-03-06T09:57:33Z

ITGA7.py

@@ -0,0 +1,206 @@
+from __future__ import annotations


обратная совместимость? здорово

NSapozhnikov · 2024-03-06T10:01:44Z

ITGA7.py

+        return self.sequence[index]
+
+    def __str__(self) -> str:
+        return f"{self.sequence}"


оригинально, но я бы сделал все же

Suggested change

return f"{self.sequence}"

return str(self.sequence)

Но ведь sequence это уже str

NSapozhnikov · 2024-03-06T17:14:10Z

ITGA7.py

+            return self.__class__(complemented_sequence)
+        except AttributeError as e:
+            raise NotImplementedError(
+                "Complement method not implemented for this class."


честно говоря, я не очень понял суть ошибки, исходя из ее текста. да и в коде выглядит странно: есть класс, в нем есть метод, но при этом эксепт говорит, что метода нет

NSapozhnikov · 2024-03-06T17:20:57Z

ITGA7.py

+        gc_symbols = set("GCgc")
+        gc_count = sum(1 for nucleotide in self.sequence if nucleotide in gc_symbols)


можно и так, но можно и сяк:

Suggested change

gc_symbols = set("GCgc")

gc_count = sum(1 for nucleotide in self.sequence if nucleotide in gc_symbols)

gc_count = (self.sequence.count('G') +

self.sequence.count('C') +

self.sequence.count('g') +

self.sequence.count('c'))

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

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

rereremin · 2024-03-08T11:08:18Z

ITGA7.py

+    def __init__(self, sequence: str):
+        self.sequence = sequence
+
+    def __len__(self) -> int:
+        return len(self.sequence)
+
+    def __getitem__(self, index: int) -> str:
+        return self.sequence[index]
+
+    def __str__(self) -> str:
+        return f"{self.sequence}"


Dunder'ы можно не аннотировать, но и в их аннотации ничего критичного нет.

Ну да, это не имеет смысла, так что лучше не надо, потому что кто-то может не очень понять зачем так делать.

rereremin · 2024-03-08T11:17:07Z

ITGA7.py

+        self.COMPLEMENT_MAP = {
+            "A": "T",
+            "C": "G",
+            "G": "C",
+            "T": "A",
+            "a": "t",
+            "c": "g",
+            "g": "c",
+            "t": "a",
+        }


Мне кажется, что удобнее записывать буквы в верхнем регистре при их перечислении в константах (gc_symbols, self.ALPHABET, self.COMPLEMENT_MAP и т.д.). Поступающую строку, даже если она в разных регистрах, можно приводить к одному уже внутри методов.
Но это дело вкуса, конечно)

Тут это теперь не константа. Константы это переменные которые объявляются в самом начале скрипта. Это просто классовый атрибут, поэтому его не надо делать капсом.

rereremin · 2024-03-08T11:24:39Z

ITGA7.py

+    filtered_records = []
+
+    for record in records:
+        gc_content = gc_fraction(record.seq)


В этой ситуации можно использовать и другой метод (GC) из класса Bio.SeqUtils, осуществляющий подсчет GC-состава:

Suggested change

gc_content = gc_fraction(record.seq)

gc_content = Bio.SeqUtils.GC(record.seq)

rereremin · 2024-03-08T11:33:14Z

Благодарю за выполненную работу!
Мои комментарии несли исключительно косметический характер, каких-то серьезных недочетов я не нашел и не увидел. Думаю, это можно считать комплиментом в Вашу сторону)

uzunmasha

Привет! Очень классный код!
Ничего серьезного добавить к предыдущим комментариям не могу, нашла только пару мелочей: в транскрипции РНК, наверное, лучше все буквы делать одного регистра, и у меня не прошла проверка на аминокислотный алфавит.
А так все отлично! Спасибо за красивый код :)

uzunmasha · 2024-03-10T15:12:16Z

ITGA7.py

@@ -0,0 +1,206 @@
+from __future__ import annotations


Это я возьму себе на заметку. Спасибо :)

uzunmasha · 2024-03-10T15:17:23Z

ITGA7.py

+
+    def __init__(self, sequence: str):
+        super().__init__(sequence)
+        self.ALPHABET = set("GgLlYySsEeQqDdNnFfAaKkRrHhCcVvPpWwIiMmTt")


Я вызвала count_aa с символами BjouXZ, и мне выдало ответ вместо ошибки, что такие символы - не аминокислоты. Возможно, стоит доработать вывод ошибок.

Супер что прогоняешь код!

Add ITGA7.py

88f72a7

NSapozhnikov reviewed Mar 6, 2024

View reviewed changes

rereremin reviewed Mar 8, 2024

View reviewed changes

uzunmasha reviewed Mar 10, 2024

View reviewed changes

		gc_symbols = set("GCgc")
		gc_count = sum(1 for nucleotide in self.sequence if nucleotide in gc_symbols)

-        gc_symbols = set("GCgc")
-        gc_count = sum(1 for nucleotide in self.sequence if nucleotide in gc_symbols)
+        gc_count = (self.sequence.count('G') +
+                    self.sequence.count('C') +
+                    self.sequence.count('g') +
+                    self.sequence.count('c'))

	gc_content = gc_fraction(record.seq)
	gc_content = Bio.SeqUtils.GC(record.seq)

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

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

rereremin Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rereremin commented Mar 8, 2024

Uh oh!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rereremin Mar 8, 2024 •

edited

Loading