Review GAGE5 by nvaulin · Pull Request #42 · Python-BI-2023/Peer_review

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

Review GAGE5

alibibio

В целом все хорошо, молодец!
Есть пара замечаний в комментариях. А также желательно сделать обработку исключений.
В задании "Biological sequences world". Не обработан крайний случай: длина последовательностей 0.

alibibio · 2024-03-03T13:27:38Z

GAGE5.py

+    elif not output_filename.endswith('.fastq'):
+        output_filename += '.fastq'


alibibio · 2024-03-03T13:31:30Z

GAGE5.py

+                filtered_seqs.append(record)
+
+    if output_filename is None:
+        output_filename = f"filtered_{input_path}"


Если в переменной input_path будет путь с использованием / или :, то такое имя файла будет невалидным.

alibibio · 2024-03-03T14:22:32Z

GAGE5.py

+    def complement(self):
+        """Return the complement sequence."""
+        comp_map_dna = {"A": "T", "G": "C", "T": "A", "C": "G", "a": "t", "t": "a", "g": "c", "c": "g"}
+        return ''.join(comp_map_dna.get(base, base) for base in self.sequence)


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

Верно подмечено. Здорово тогда было бы добавить что это решается использованием не get а [...]. В целом через get я очень редко встречал чтобы делали

alibibio · 2024-03-03T14:28:19Z

GAGE5.py

+    def transcribe(self):
+        """Transcribe the DNA sequence into an RNA sequence."""
+        transcribed_seq = ''.join(self.TRANSCRIBE_DICT.get(base, base) for base in self.sequence)
+        return transcribed_seq


сначала требует вызова complement(), а затем замену по словарю TRANSCRIBE_DICT

alibibio · 2024-03-03T14:36:23Z

GAGE5.py

+
+    def check_alphabet(self):
+        """Check if the sequence contains valid amino acid alphabet characters."""
+        if not set(self.sequence).issubset(NucleicAcidSequence.AMINO_ACID_LETTERS):


Данный set лучше вынести из класса NucleicAcidSequence

Olga-Bagrova

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

Бонус поразвлекаться. Запустите это: print("\u2764\uFE0F")

Olga-Bagrova · 2024-03-06T12:42:27Z

GAGE5.py

+from Bio.SeqUtils import GC
+from abc import ABC, abstractmethod
+
+def filter_fastq(input_path, gc_bounds=(0, 100), length_bounds=(0, float('inf')), quality_threshold=0, output_filename=None):


Переменные могут загрустить, что не знают свои типы здесь (но они точно есть в докстринге и это хорошо):

Suggested change

def filter_fastq(input_path, gc_bounds=(0, 100), length_bounds=(0, float('inf')), quality_threshold=0, output_filename=None):

def filter_fastq(input_path: str, gc_bounds: tuple = (0, 100), length_bounds: tuple = (0, float('inf')), quality_threshold: float = 0, output_filename: str = None)->str:

Olga-Bagrova · 2024-03-06T12:55:13Z

GAGE5.py

+    """
+    Filters a FASTQ file based on GC content, sequence length, and quality threshold using Biopython.
+
+    Args:
+    - input_path (str): Path to the input FASTQ file.
+    - gc_bounds (tuple): Tuple specifying the minimum and maximum GC content for filtering. Default is (0, 100).
+    - length_bounds (tuple): Tuple specifying the minimum and maximum sequence length for filtering. Default is (0, infinity).
+    - quality_threshold (float): Minimum quality score for filtering. Default is 0.
+    - output_filename (str): Name of the output file. If None, the default filename will be used.
+
+    Returns:
+    - str: Message indicating the success of the filtering process.
+    """


Очень классная докстринга: понятно что делает функция, какие аргументы, их типы, а также дефолтные значения.

Olga-Bagrova · 2024-03-06T13:15:10Z

GAGE5.py

+
+    def gc_content(self):
+        """Return the GC content of the sequence."""
+        gc_count = (self.sequence.upper().count('G') + self.sequence.upper().count('C')) / len(self.sequence) * 100


Учтено, что могут быть заглавные и прописные буквы. Класс!

Olga-Bagrova · 2024-03-06T13:19:46Z

GAGE5.py

+        """Initialize a DNASequence object with a given sequence."""
+        super().__init__(sequence)
+        if not self.check_alphabet():
+            raise ValueError("Invalid DNA sequence")


Круто, что есть встроенная проверка. Но если взять что-то типо DNASequence('AUTCWT'), то тоже пройдёт проверку на алфавит, хотя тут есть и U (РНК), и T (ДНК), и W (Белок).

Верно подмечено

Olga-Bagrova · 2024-03-06T13:35:16Z

GAGE5.py

+
+    DNA_LETTERS = set("ATGCatgc")
+    RNA_LETTERS = set("AUGCaugc")
+    AMINO_ACID_LETTERS = set("ACDEFGHIKLMNPQRSTVWY")


Возможно аминокислотный алфавит стоило вынести из класса для нуклеиновых кислот, чтобы наши абстракции не нарушали биологию 😅. В целом классно, что учтено наличие больших и маленьких букв.

Olga-Bagrova · 2024-03-06T13:40:50Z

GAGE5.py

+    def complement(self):
+        """Return the complement sequence."""
+        comp_map_dna = {"A": "T", "G": "C", "T": "A", "C": "G", "a": "t", "t": "a", "g": "c", "c": "g"}
+        return ''.join(comp_map_dna.get(base, base) for base in self.sequence)


Подкину к комментарию про метод get для словарей возможную альтернативу. Хотя конечно в дочерних классах и так идёт проверка алфавита при инициализации - это продуманно).

Suggested change

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

return ''.join([comp_map_dna[base] for base in self.sequence])

Olga-Bagrova · 2024-03-06T13:43:26Z

GAGE5.py

+        total_length = len(self.sequence)
+        for group, count in profile.items():
+            profile[group] = round((count / total_length), 2)
+        return profile


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

Olga-Bagrova · 2024-03-06T14:09:17Z

GAGE5.py

+    def check_alphabet(self):
+        """Check if the sequence contains valid amino acid alphabet characters."""
+        if not set(self.sequence).issubset(NucleicAcidSequence.AMINO_ACID_LETTERS):
+            raise ValueError("Invalid amino acid sequence")


Если просто запустить AminoAcidSequence('QWERTY').check_alphabet(), то ничего не вернёт. Для ДНК и РНК было классно сделано с return'ом булевого значения.

Cucumberan

Отличный код, прекрасная работа!
Кроме нескольких проверок на правильность вводимых последовательностей (на них указали ревьюеры до меня), все сделано замечательно, исправлять и предлагать мне нечего. Оставила лишь пару комментариев о том, как можно по-другому. А так все супер.

Cucumberan · 2024-03-10T08:24:42Z

GAGE5.py

+
+    def reverse(self):
+        """Return the reverse of the RNA sequence."""
+        return RNASequence(self.sequence[::-1])


Здесь также, как и в предыдущих случаях, можно возвращать просто развернутую последовательность, а не создавать новый экземпляр класса

Suggested change

return RNASequence(self.sequence[::-1])

return self.sequence[::-1]

Ну не совсем.

У нас был объект сиквенс. Мы его развернули. Почему в ходе разворота тип данных изменила и последовательность превратилась в строку? Хотя тут проблема с тем что захардкожено имя класса, лучше через type(self)

Cucumberan · 2024-03-10T08:30:37Z

GAGE5.py

+
+    def check_alphabet(self):
+        """Check if the sequence contains valid nucleic- or aminoacid alphabet characters."""
+        return set(self.sequence).issubset(self.DNA_LETTERS | self.RNA_LETTERS | self.AMINO_ACID_LETTERS)


Мне кажется, что проверку на принадлежность к аминокислотной последовательности можно было бы убрать из класса NucleicAcidSequence и оставить только в классе AminoAcidSequence.

check_alphabet должа быть у всех из них
но про наборы букв - справедливо. Лучше иметь просто одну переменную - классовый атрибут alphabet с которым мы будем сравивать, но у разных классов он будет разный

Cucumberan · 2024-03-10T08:37:08Z

GAGE5.py

+    def transcribe(self):
+        """Transcribe the DNA sequence into an RNA sequence."""
+        transcribed_seq = ''.join(self.TRANSCRIBE_DICT.get(base, base) for base in self.sequence)
+        return transcribed_seq


Здесь в методе transcribe возвращается просто строка transcribed_seq, а ниже в классе RNASequence при вызове метода reverse возращается новый объект класса RNASequence. Можно было бы для достижения единообразия и там и там возращать строки, либо чтобы после вызова метода transcribe создавался объект класса RNASequence.

Suggested change

return transcribed_seq

return RNASequence(transcribed_seq)

nvaulin · 2024-03-17T19:52:17Z

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

Бонус поразвлекаться. Запустите это: print("\u2764\uFE0F")

Ахахах
Запомните, дети, не запускайте непонятный код из интернетов

Add GAGE5.py

a70b466

alibibio reviewed Mar 3, 2024

View reviewed changes

Olga-Bagrova reviewed Mar 6, 2024

View reviewed changes

Cucumberan reviewed Mar 10, 2024

View reviewed changes

		elif not output_filename.endswith('.fastq'):
		output_filename += '.fastq'

	def filter_fastq(input_path, gc_bounds=(0, 100), length_bounds=(0, float('inf')), quality_threshold=0, output_filename=None):
	def filter_fastq(input_path: str, gc_bounds: tuple = (0, 100), length_bounds: tuple = (0, float('inf')), quality_threshold: float = 0, output_filename: str = None)->str:

	return ''.join(comp_map_dna.get(base, base) for base in self.sequence)
	return ''.join([comp_map_dna[base] for base in self.sequence])

	return RNASequence(self.sequence[::-1])
	return self.sequence[::-1]

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

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

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!

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

nvaulin commented Mar 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants