Review TOMM20 by nvaulin · Pull Request #31 · Python-BI-2023/Peer_review

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

Review TOMM20

NSapozhnikov

Все здорово, очень чисто и аккуратно. Мне понравилось!
Из нового смог лишь предложить пару "оптимизаций", возможно, не вполне уместных, но это скорее, чтобы показать, как можно еще нетривиально :)

NSapozhnikov · 2024-03-06T09:26:39Z

TOMM20.py

+        if self.check_seq_type('DNA'):
+            self.seq_type = 'DNA'
+        elif self.check_seq_type('RNA'):
+            self.seq_type = 'RNA'
+        elif self.check_seq_type('Protein'):
+            self.seq_type = 'Protein'
+        else:
+            raise ValueError(f'Sequence {self.seq} can not be analysed!')


Можно еще вот так, и когда сравнение больше чем из 2х случаев, работает чуть быстрее

Suggested change

if self.check_seq_type('DNA'):

self.seq_type = 'DNA'

elif self.check_seq_type('RNA'):

self.seq_type = 'RNA'

elif self.check_seq_type('Protein'):

self.seq_type = 'Protein'

else:

raise ValueError(f'Sequence {self.seq} can not be analysed!')

match True:

case self.check_seq_type('DNA'):

self.seq_type = 'DNA'

case self.check_seq_type('RNA'):

self.seq_type = 'RNA'

case self.check_seq_type('Protein'):

self.seq_type = 'Protein'

case _:

raise ValueError(f'Sequence {self.seq} can not be analysed!')

NSapozhnikov · 2024-03-06T09:32:51Z

TOMM20.py

+        for nucleotide in self.seq:
+            output_seq.append(self.complement_alphabet[nucleotide])
+        return NucleicAcidSequence(''.join(output_seq))


функция map() должна работать чуть побыстрее

Suggested change

for nucleotide in self.seq:

output_seq.append(self.complement_alphabet[nucleotide])

return NucleicAcidSequence(''.join(output_seq))

output_seq = ''.join(map(lambda nucleotide: self.complement_alphabet[nucleotide], self.seq))

return NucleicAcidSequence(output_seq)

Ну вот мы только сейчас прошли map и lambda :)
Но да, так более функционально

NSapozhnikov · 2024-03-06T09:35:26Z

TOMM20.py

+        gc_result = (self.seq.count('C') + self.seq.count('G')) / len(self.seq) * 100
+        return round(gc_result, 3)


классно!

NSapozhnikov · 2024-03-06T09:38:14Z

TOMM20.py

+        output_seq = self.seq.replace('T', 'U').replace('t', 'u')
+        return RNASequence(output_seq)


NSapozhnikov · 2024-03-06T09:41:44Z

TOMM20.py

+        if not super().check_seq_type('Protein'):
+            raise ValueError(f'Sequence {self.seq} is not protein')


как будто немного странно делать повторные проверки в дочерних классах, ведь в родительском классе уже была проверка. получается, если последовательность прошла первую проверку (что это ДНК, РНК или протеин), то дальше все должно работать, а тут вдруг она дальше неожиданно решит упасть, так как оказалось, что ДНК != РНК

NSapozhnikov · 2024-03-06T09:51:47Z

TOMM20.py

+    if output_filename is None:
+        output_filename = 'filtered_fastq'
+
+    with open(input_path) as handle, open(os.path.join(output_data_dir, output_filename), mode='w') as file:


о, не знал, что можно открывать через запятую в одном блоке with

Olga-Bagrova

Приятно было смотреть на код. В докстрингах всё аккуратно описано. Немного оставила своих предложений по коду, но они не критические. В некоторых местах показалось, что код избыточен. Но всё запускается и работает правильно. Удачи!

Предлагаю поразвлекаться, запустив это: print('\n'.join(' '.join(*zip(*row)) for row in ([["*" if row==0 and col%3!=0 or row==1 and col%3==0 or row-col==2 or row+col==8 else " " for col in range(7)] for row in range(6)])))

Olga-Bagrova · 2024-03-06T14:30:40Z

TOMM20.py

+def filter_fastq(input_path: str, gc_bounds: tuple or int = (0, 100),
+                 length_bounds: tuple or int = (0, 2 ** 32),
+                 quality_threshold: int = 0,
+                 output_filename=None,
+                 output_data_dir: str = 'filter_fastq_results'):


Аннотация типов есть - класс! Понравилось, что тут аккуратно всё оформленно.

Olga-Bagrova · 2024-03-06T14:33:08Z

TOMM20.py

+    if not os.path.isdir(output_data_dir):
+        os.mkdir(output_data_dir)


Круто, что отсутствие папки продуманно

Olga-Bagrova · 2024-03-06T14:38:01Z

TOMM20.py

+            if type(length_bounds) != tuple:
+                length_bounds = tuple([0, length_bounds])


Проверку типов можно было вынести из цикла for в тело функции filter_fastq, чтобы сразу один раз проверить и подправить всё. На времени работы, наверно, не сильно сказывается.

Olga-Bagrova · 2024-03-06T14:40:31Z

TOMM20.py

+                out = 1
+
+            if out == 0:
+                file.write(lin.format("fastq"))


Оригинально сделано с переменной out. Даже, считай, дискретка использовалась, чтобы с логическими выражениями поработать)

Olga-Bagrova · 2024-03-06T14:43:38Z

TOMM20.py

+ALPHABET_FOR_DNA = {'A', 'T', 'G', 'C', 'a', 't', 'g', 'c'}
+ALPHABET_FOR_RNA = {'A', 'U', 'G', 'C', 'a', 'u', 'g', 'c'}
+ALPHABET_FOR_PROTEIN = set('FLIMVSPTAYHQNKDECWRG')


Если занудствовать, то можно было унифицировать и сделать всё через set.
Но это конечно "Откройте окно! Слишком душно"

Suggested change

ALPHABET_FOR_DNA = {'A', 'T', 'G', 'C', 'a', 't', 'g', 'c'}

ALPHABET_FOR_RNA = {'A', 'U', 'G', 'C', 'a', 'u', 'g', 'c'}

ALPHABET_FOR_PROTEIN = set('FLIMVSPTAYHQNKDECWRG')

ALPHABET_FOR_DNA = set('ATGCatgc')

ALPHABET_FOR_RNA = set('AUGCaugc')

ALPHABET_FOR_PROTEIN = set('FLIMVSPTAYHQNKDECWRG')

Olga-Bagrova · 2024-03-06T14:56:02Z

TOMM20.py

+        Return:
+        -   sequence gc content, %
+        """
+        gc_result = (self.seq.count('C') + self.seq.count('G')) / len(self.seq) * 100


Строго говоря, в алфавите есть и прописные буквы. И в данном случае маленькие c и g не посчитаются (но у меня, если честно, также было).

Suggested change

gc_result = (self.seq.count('C') + self.seq.count('G')) / len(self.seq) * 100

gc_result = (self.seq.upper().count('C') + self.upper().seq.count('G')) / len(self.seq) * 100

Olga-Bagrova · 2024-03-06T15:01:45Z

TOMM20.py

+        return:
+        -   RNA sequence
+        """
+        output_seq = self.seq.replace('T', 'U').replace('t', 'u')


Выглядит лаконично. «Краткость – сестра таланта».

Olga-Bagrova · 2024-03-06T15:03:37Z

TOMM20.py

+    def counting_molecular_weight(self):
+        """
+        Counts the molecular mass of a protein sequence seq
+        Arguments:
+        - seq (str): sequence to count the molecular weight
+        Return:
+        - output (int): molecular weight value
+        """
+        output = 0
+        for amino_acid in self.seq:
+            output += DICT_MOLECULAR_MASS[amino_acid]
+        return output - 18 * (len(self.seq) - 1)


Классный метод!

Olga-Bagrova · 2024-03-06T15:08:41Z

TOMM20.py

+    if output_filename is None:
+        output_filename = 'filtered_fastq'
+
+    with open(input_path) as handle, open(os.path.join(output_data_dir, output_filename), mode='w') as file:


тоже не знала про with с запятыми)

Olga-Bagrova · 2024-03-06T15:11:17Z

TOMM20.py

+        if super().check_seq_type('DNA'):
+            self.complement_alphabet = COMPLEMENT_ALPHABET_DNA
+        elif super().check_seq_type('RNA'):


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

artyomtorr

Очень хорошая работа! Могу похвалить за интересную реализацию fastq-фильтратора, подробную аннотацию типов и докстринги, обилие методов в классах биологических последовательностей.
Из идей для оптимизации прежде всего хочу предложить более активное использование полиморфных методов и атрибутов, что поможет избавиться от дублирования в дочерних классах.

artyomtorr · 2024-03-07T20:43:26Z

TOMM20.py

+ALPHABET_FOR_DNA = {'A', 'T', 'G', 'C', 'a', 't', 'g', 'c'}
+ALPHABET_FOR_RNA = {'A', 'U', 'G', 'C', 'a', 'u', 'g', 'c'}
+ALPHABET_FOR_PROTEIN = set('FLIMVSPTAYHQNKDECWRG')
+COMPLEMENT_ALPHABET_DNA = {'A': 'T', 'T': 'A', 'G': 'C', 'C': 'G',
+                           'a': 't', 't': 'a', 'g': 'c', 'c': 'g'}
+COMPLEMENT_ALPHABET_RNA = {'U': 'A', 'A': 'U', 'G': 'C', 'C': 'G',
+                           'u': 'a', 'a': 'u', 'g': 'c', 'c': 'g'}


Будет совсем замечательно, если все эти глобальные переменные превратить в полиморфные атрибуты классов DNASequence и RNASequence. Тогда бы они имели одинаковые названия (например, ALPHABET и COMPLEMENT_ALPHABET без всяких приставок), но разное содержание в зависимости от класса.
Благодаря этому, в функции check_seq_type можно было бы обойтись без проверки типа последовательности

artyomtorr · 2024-03-07T20:56:13Z

TOMM20.py

+        return self.seq[start: end]
+
+    def __repr__(self):
+        return f'The sequence is: {self.seq}, type is {self.seq_type}'


су-пер!

artyomtorr · 2024-03-07T20:59:32Z

TOMM20.py

+
+    def __init__(self, seq):
+        self.seq = seq
+        self.seq_type = None


В целом... создание объектов BiologicalSequence (как и NucleicAcidSequence), на мой взгляд, не предполагается, а для объектов DNASequence, RNASequence и AminoAcidSequence название класса говорит само за себя.

Suggested change

self.seq_type = None

artyomtorr · 2024-03-07T21:00:21Z

TOMM20.py

+}
+
+
+class BiologicalSequence(str):


Абстрактные классы обычно наследуются от базового класса ABC из модуля abc. Но конкретно в этом случае, наследование от str выглядит вполне логичным :)

Suggested change

class BiologicalSequence(str):

from abc import ABC

class BiologicalSequence(ABC):

artyomtorr · 2024-03-07T21:06:13Z

TOMM20.py

+        -   RNA sequence
+        """
+        output_seq = self.seq.replace('T', 'U').replace('t', 'u')
+        return RNASequence(output_seq)


artyomtorr · 2024-03-07T21:11:24Z

TOMM20.py

+        output_seq = []
+        for nucleotide in self.seq:
+            output_seq.append(self.complement_alphabet[nucleotide])
+        return NucleicAcidSequence(''.join(output_seq))


Так мы сможем возвращать объект того же класса, который принимаем на вход:

Suggested change

return NucleicAcidSequence(''.join(output_seq))

return type(self)(''.join(output_seq))

Опять же, если считаем, что класс NucleicAcidSequence мы используем для наследования, а не для создания объектов

artyomtorr · 2024-03-07T21:15:04Z

TOMM20.py

+    if output_filename is None:
+        output_filename = 'filtered_fastq'
+
+    with open(input_path) as handle, open(os.path.join(output_data_dir, output_filename), mode='w') as file:


присоединяюсь к комментариям, очень продуманно!

artyomtorr · 2024-03-07T21:18:03Z

TOMM20.py

+        record = SeqIO.parse(handle, "fastq")
+        for lin in record:


Скорее придирка, но такие названия кажутся более логичными

Suggested change

record = SeqIO.parse(handle, "fastq")

for lin in record:

records = SeqIO.parse(handle, "fastq")

for record in records:

nvaulin added 2 commits February 26, 2024 20:50

Add TOMM20.py

6485e63

Add TOMM20.py

fa0eaef

NSapozhnikov reviewed Mar 6, 2024

View reviewed changes

Olga-Bagrova reviewed Mar 6, 2024

View reviewed changes

artyomtorr reviewed Mar 7, 2024

View reviewed changes

		gc_result = (self.seq.count('C') + self.seq.count('G')) / len(self.seq) * 100
		return round(gc_result, 3)

		output_seq = self.seq.replace('T', 'U').replace('t', 'u')
		return RNASequence(output_seq)

		if not super().check_seq_type('Protein'):
		raise ValueError(f'Sequence {self.seq} is not protein')

		if not os.path.isdir(output_data_dir):
		os.mkdir(output_data_dir)

		if type(length_bounds) != tuple:
		length_bounds = tuple([0, length_bounds])

	gc_result = (self.seq.count('C') + self.seq.count('G')) / len(self.seq) * 100
	gc_result = (self.seq.upper().count('C') + self.upper().seq.count('G')) / len(self.seq) * 100

-class BiologicalSequence(str):
+from abc import ABC
+class BiologicalSequence(ABC):

	return NucleicAcidSequence(''.join(output_seq))
	return type(self)(''.join(output_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!

Olga-Bagrova 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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants