Review NPAS1 by nvaulin · Pull Request #43 · Python-BI-2023/Peer_review

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

Review NPAS1

rereremin

Спасибо за выполненную работу!

Мне очень понравились некоторые Ваши решения, которые я возьму себе на заметку. Также отмечу высокое качество кода, однако еще бы аннотации типов не помешало добавить, тогда точно будет шик-блекс!

rereremin · 2024-03-08T11:40:23Z

NPAS1.py

+
+

Здесь можно было написать функцию, которая осуществляет трансляцию. Вероятно, Вы просто не успели что-либо написать в этом классе.

rereremin · 2024-03-08T11:40:29Z

NPAS1.py

+    @abstractmethod
+    def __len__(self):
+        pass
+
+    @abstractmethod
+    def __getitem__(self, index):


Отличное использование декораторов!

rereremin · 2024-03-08T11:42:02Z

NPAS1.py

+        gc_content = gc_count / total_count if total_count > 0 else 0
+        return gc_content * 100 if as_percentage else gc_content


Круто, что не забыли про пустые строки и деление на ноль!

rereremin · 2024-03-08T11:43:16Z

NPAS1.py

+        rna_seq = ""
+        for aa in self.sequence:
+            codon = choice(AA_CODON_DICT[aa])
+            rna_seq += codon
+        return rna_seq


А, Вы решили восстанавливать мРНК, тоже хорошая идея.

anisssum

Здорово, что использовали абстрактные классы (ABC) для представления биологических последовательностей.
Код соблюдает стандарты PEP 8, что способствует единообразию и читаемости.

anisssum · 2024-03-10T05:43:34Z

NPAS1.py

+        if gc_bounds[0] <= gc_percent <= gc_bounds[1] and \
+                length_bounds[0] <= seq_len <= length_bounds[1] and \


Можно было бы еще учесть. что gc_bounds и length_bounds может подаваться одно число, являющееся верхней границей.

Даже не можно а нужно :)

anisssum · 2024-03-10T05:50:14Z

NPAS1.py

+        output_filename = input_path.split("/")[-1]
+    else:
+        output_filename = output_filename + ".fastq"


Если в input_path уже написано расширение у файла, то он к этому расширению добавит ".fastq". Также некоторые операционные системы могут не понять "/".
Кажется, лучше сплитовать по ".", брать элемент с индексом 0 и добавлять к нему ".fasta". Тогда output файл будет сохранен в той же папке с одним расширением.

Верное замечание. А вообще чтобы было универсально можно использовать basename

anisssum · 2024-03-10T05:54:23Z

NPAS1.py

+        return self.sequence[index]
+
+    def complement(self):
+        return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)


Круто написано.
Можно было бы еще учесть, что на вход подается РНК.

anisssum · 2024-03-10T05:56:02Z

NPAS1.py

+    def complement(self):
+        return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)
+
+    def gc_content(self, as_percentage=False):


Здорово, что есть вариант с процентами "as_percentage".

wwoskie

Доброе утро! Поздравляю с прошедшим междунородным днем проверки второй домашки!

Работа хорошая!

Концептуально на мой взгляд все верно. Докстринги есть, это хорошо. Немного обидно, что некоторые методы возвращают не свой класс, а str. Еще по удобству чтения кода: мне кажется, читать аннотацию типов из докстрингов не так удобно, как из самих аргументов функции (ну хотя подцветки синтаксиса нет), да и автопроверки это тоже делают оттуда, поэтому я бы все-таки добавил их при объявлении функции

wwoskie · 2024-03-10T23:54:48Z

NPAS1.py

+        if gc_bounds[0] <= gc_percent <= gc_bounds[1] and \
+                length_bounds[0] <= seq_len <= length_bounds[1] and \
+                mean_offset >= quality_threshold:


Suggested change

if gc_bounds[0] <= gc_percent <= gc_bounds[1] and \

length_bounds[0] <= seq_len <= length_bounds[1] and \

mean_offset >= quality_threshold:

if (

gc_bounds[0] <= gc_percent <= gc_bounds[1]

and length_bounds[0] <= seq_len <= length_bounds[1]

and mean_offset >= quality_threshold

):

Есть такой ЛаЙфХаК, хотя твой способ не осуждаю (но этот имо аккурпатнее)

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

wwoskie · 2024-03-10T23:56:18Z

NPAS1.py

+                mean_offset >= quality_threshold:
+            filtered_seqs.append(record)
+
+    if output_filename is None:


Хорошо, что проверка есть

wwoskie · 2024-03-10T23:58:27Z

NPAS1.py

+            filtered_seqs.append(record)
+
+    if output_filename is None:
+        output_filename = input_path.split("/")[-1]


Я бы предложил для таких проверок использовать всё-таки библиотечки типа os или pathlib

wwoskie · 2024-03-11T00:00:01Z

NPAS1.py

+    if output_filename is None:
+        output_filename = input_path.split("/")[-1]
+    else:
+        output_filename = output_filename + ".fastq"


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

wwoskie · 2024-03-11T00:03:55Z

NPAS1.py

+        if not self.alphabet_checking():
+            raise ValueError("Invalid characters in the sequence.")
+
+    @abstractmethod


Круто, что ты делаешь самостоятельно эти базовые методы

wwoskie · 2024-03-11T00:09:54Z

NPAS1.py

+        return self.sequence[index]
+
+    def complement(self):
+        return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)


Интересная конструкция

Хотя это и немного странно, что он вернет сам себя, как будто немного неинтуитивное поведение

wwoskie · 2024-03-11T00:11:40Z

NPAS1.py

+        return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)
+
+    def gc_content(self, as_percentage=False):
+        gc_count = self.sequence.count('G') + self.sequence.count('C')


count в этом случае пробежится два раза, если будет длинная последовательность может быть неприятно.

wwoskie · 2024-03-11T00:13:33Z

NPAS1.py

+    TRANSCRIBE_DICT = {
+        'T': 'U',
+        't': 'u'
+    }


Нативная интеграция (извините)

wwoskie · 2024-03-11T00:16:00Z

NPAS1.py

+        return f"{self.__class__.__name__}('{self.sequence}')"
+
+    def alphabet_checking(self):
+        if not set(self.sequence) <= set(type(self).ALPHABET):


wwoskie · 2024-03-11T00:21:04Z

NPAS1.py

+        return self.sequence[index]
+
+    def complement(self):
+        return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)


Suggested change

return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)

return self.__class__(''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence))

можно как-то так попробовать, а то результат выполнения возвращает строку, что не совсем хорошо

Чаще делают не self.__class__(...), а type(self)(...)

Add NPAS1.py

638b0e1

rereremin reviewed Mar 8, 2024

View reviewed changes

anisssum reviewed Mar 10, 2024

View reviewed changes

wwoskie reviewed Mar 11, 2024

View reviewed changes

		gc_content = gc_count / total_count if total_count > 0 else 0
		return gc_content * 100 if as_percentage else gc_content

		if gc_bounds[0] <= gc_percent <= gc_bounds[1] and \
		length_bounds[0] <= seq_len <= length_bounds[1] and \

	return ''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence)
	return self.__class__(''.join(self.COMPLEMENT_DICT.get(base, base) for base in self.sequence))

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

rereremin left a comment • 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!

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!

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

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

wwoskie Mar 11, 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!

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

rereremin left a comment •

edited

Loading

wwoskie Mar 11, 2024 •

edited

Loading