Review ELMOD1 by nvaulin · Pull Request #38 · Python-BI-2023/Peer_review

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

Review ELMOD1

Alisa411

Хорошая работа! Пока проверяла ее, поняла, где сама накосячила в своем коде!
Я рада, что получилось в целом разобраться, единственное, я бы посоветовала еще раз вспомнить метод super() и посмотреть на "избыточность кода" с методом is_valid_alphabet.
Здорово, что удалось везде прописать типы данных и докстринги, в разы облегчило проверку!

Alisa411 · 2024-03-06T17:35:57Z

ELMOD1.py

+    def __init__(self, input_path: str, output_filename: str, gc_bounds: Union[int, Tuple[int, int]] = (0, 100),
+                 length_bounds: Union[int, Tuple[int, int]] = (0, 2**32), quality_threshold: int = 0):


Классно, что по PEP8!

Alisa411 · 2024-03-06T17:36:55Z

ELMOD1.py

+from Bio import SeqIO
+from Bio.SeqUtils import gc_fraction
+from Bio.SeqRecord import SeqRecord
+from typing import List, Dict, Union, Tuple
+from abc import ABC, abstractmethod
+import os


Секунда духоты. По PEP8 правильнее так, но это не критично совсем :)

Suggested change

from Bio import SeqIO

from Bio.SeqUtils import gc_fraction

from Bio.SeqRecord import SeqRecord

from typing import List, Dict, Union, Tuple

from abc import ABC, abstractmethod

import os

from typing import List, Dict, Union, Tuple

from abc import ABC, abstractmethod

from Bio import SeqIO

from Bio.SeqUtils import gc_fraction

from Bio.SeqRecord import SeqRecord

Совершенно верно, ревью это то место где душнить можно и нужно
Я бы еще пустую строку поставил между встроенными и сторонними либами

Alisa411 · 2024-03-06T18:10:03Z

ELMOD1.py

+        self.gc_bounds = gc_bounds
+        self.length_bounds = length_bounds
+        self.quality_threshold = quality_threshold
+


Было бы здорово прописать проверку вводного файла:

Suggested change

if not os.path.isfile(self.input_path):

raise FileNotFoundError(f"Input file '{self.input_path}' not found.")

Alisa411 · 2024-03-06T18:21:49Z

ELMOD1.py

+        records = list(SeqIO.parse(self.input_path, "fastq"))
+        return records
+
+    def apply_filters(self, records: List[SeqRecord]) -> List[SeqRecord]:


Здорово, что прописаны типы данных везде!

Alisa411 · 2024-03-06T18:25:05Z

ELMOD1.py

+        if not self.is_valid_alphabet():
+            raise ValueError("Invalid alphabet for {}: {}".format(self.__class__.__name__, str(self)))


Хорошо, что вызывается ошибка, в случае если подается несуществующая последовательность

Alisa411 · 2024-03-06T19:03:57Z

ELMOD1.py

+        Returns:
+        - float: GC content of the sequence.
+        """
+        self.validate_alphabet()


Аналогично предыдущему комментарию

Suggested change

self.validate_alphabet()

Alisa411 · 2024-03-06T19:05:49Z

ELMOD1.py

+        Returns:
+        - RNASequence: Transcribed RNA sequence.
+        """
+        self.validate_alphabet()


Аналогично предыдущему комментарию

Suggested change

self.validate_alphabet()

Alisa411 · 2024-03-06T19:10:32Z

ELMOD1.py

+        Returns:
+        - List[Dict[str, int]]: List of dictionaries containing Brutto formula values for each amino acid.
+        """
+        self.validate_alphabet()


Так как мы мы делаем валидацию при инициализации, нам это не нужно

Suggested change

self.validate_alphabet()

Alisa411 · 2024-03-06T19:11:46Z

ELMOD1.py

+        Returns:
+        - bool: True if the alphabet is valid, False otherwise.
+        """
+        return set(self.sequence) <= {'A', 'R', 'N', 'D', 'V', 'H', 'G', 'Q', 'E', 'I', 'L', 'K', 'M',


Так как эти буквы уже прописаны в словаре amino_brutto, можно сделать немного хитрее:

Suggested change

return set(self.sequence) <= {'A', 'R', 'N', 'D', 'V', 'H', 'G', 'Q', 'E', 'I', 'L', 'K', 'M',

return set(self) <= set(self.amino_brutto.keys())

При проверке на ключи можно даже не ставить keys(), так как словарь по умолчанию итерируется по ключам. Хотя не знаю как лучше

Alisa411 · 2024-03-06T19:14:41Z

ELMOD1.py

+    amino_brutto = {
+        "A": (3, 7, 1, 2, 0),
+        "R": (6, 14, 4, 2, 0),
+        "N": (4, 8, 2, 3, 0),
+        "D": (4, 7, 1, 4, 0),
+        "V": (5, 11, 1, 2, 0),
+        "H": (6, 9, 3, 2, 0),
+        "G": (2, 5, 1, 2, 0),
+        "Q": (5, 10, 2, 3, 0),
+        "E": (5, 9, 1, 4, 0),
+        "I": (6, 13, 1, 2, 0),
+        "L": (6, 13, 1, 2, 0),
+        "K": (6, 14, 2, 2, 0),
+        "M": (5, 11, 1, 2, 1),
+        "P": (5, 9, 1, 2, 0),
+        "S": (3, 7, 1, 3, 0),
+        "Y": (9, 11, 1, 3, 0),
+        "T": (4, 9, 11, 1, 3, 0),
+        "W": (11, 12, 2, 2, 0),
+        "F": (9, 11, 1, 2, 0),
+        "C": (3, 7, 1, 2, 1),
+    }


Я бы этот словарь добавила в init_ и так как мы вносим в него изменения, обязательно добавляем метод super()

Suggested change

amino_brutto = {

"A": (3, 7, 1, 2, 0),

"R": (6, 14, 4, 2, 0),

"N": (4, 8, 2, 3, 0),

"D": (4, 7, 1, 4, 0),

"V": (5, 11, 1, 2, 0),

"H": (6, 9, 3, 2, 0),

"G": (2, 5, 1, 2, 0),

"Q": (5, 10, 2, 3, 0),

"E": (5, 9, 1, 4, 0),

"I": (6, 13, 1, 2, 0),

"L": (6, 13, 1, 2, 0),

"K": (6, 14, 2, 2, 0),

"M": (5, 11, 1, 2, 1),

"P": (5, 9, 1, 2, 0),

"S": (3, 7, 1, 3, 0),

"Y": (9, 11, 1, 3, 0),

"T": (4, 9, 11, 1, 3, 0),

"W": (11, 12, 2, 2, 0),

"F": (9, 11, 1, 2, 0),

"C": (3, 7, 1, 2, 1),

}

def __init__(self, sequence: str):

"""

Initialize an AminoAcidSequence instance.

"""

super().__init__(sequence)

amino_brutto = {

"A": (3, 7, 1, 2, 0),

"R": (6, 14, 4, 2, 0),

"N": (4, 8, 2, 3, 0),

"D": (4, 7, 1, 4, 0),

"V": (5, 11, 1, 2, 0),

"H": (6, 9, 3, 2, 0),

"G": (2, 5, 1, 2, 0),

"Q": (5, 10, 2, 3, 0),

"E": (5, 9, 1, 4, 0),

"I": (6, 13, 1, 2, 0),

"L": (6, 13, 1, 2, 0),

"K": (6, 14, 2, 2, 0),

"M": (5, 11, 1, 2, 1),

"P": (5, 9, 1, 2, 0),

"S": (3, 7, 1, 3, 0),

"Y": (9, 11, 1, 3, 0),

"T": (4, 9, 11, 1, 3, 0),

"W": (11, 12, 2, 2, 0),

"F": (9, 11, 1, 2, 0),

"C": (3, 7, 1, 2, 1),

}

Я бы этот словарь добавила в init_ и так как мы вносим в него изменения, обязательно добавляем метод super()

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

JuliGen

Хорошая работа! Единственное, стоит обратить внимание на повторение функций is_valid_alphabet и validate_alphabet, а также на избыточное число проверок алфавита.

JuliGen · 2024-03-08T14:14:43Z

ELMOD1.py

+    def __init__(self, input_path: str, output_filename: str, gc_bounds: Union[int, Tuple[int, int]] = (0, 100),
+                 length_bounds: Union[int, Tuple[int, int]] = (0, 2**32), quality_threshold: int = 0):
+        """
+        Initialize FastQFilter instance.
+
+        Parameters:
+        - input_path (str): Path to the input FastQ file.
+        - output_filename (str): Name for the output FastQ file.
+        - gc_bounds (Union[int, Tuple[int, int]]): GC content bounds for filtering.
+        - length_bounds (Union[int, Tuple[int, int]]): Length bounds for filtering.
+        - quality_threshold (int): Quality threshold for filtering.


Круто, что есть аннотация типов и докстринга

JuliGen · 2024-03-08T14:27:43Z

ELMOD1.py

+    def read_fastq(self) -> List[SeqRecord]:
+        """
+        Read FastQ file and return a list of SeqRecord objects.
+        """
+        records = list(SeqIO.parse(self.input_path, "fastq"))


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

Совершенно верно

JuliGen · 2024-03-08T14:37:02Z

ELMOD1.py

+        os.makedirs(output_dir, exist_ok=True)
+
+        SeqIO.write(records, output_path, "fastq")
+        print(f"Filtered sequences written to {output_path}")


Очень удобно, что для результатов создается отдельная папка!

JuliGen · 2024-03-08T14:46:20Z

ELMOD1.py

+    def __len__(self) -> int:
+        """
+        Return the length of the sequence.
+
+        Returns:
+        - int: Length of the sequence.
+        """
+        return len(self.sequence)
+
+    def __getitem__(self, index: int) -> str:
+        """
+        Get the character at the specified index in the sequence.
+
+        Parameters:
+        - index (int): The index to retrieve.
+
+        Returns:
+        - str: The character at the specified index.
+        """
+        return self.sequence[index]


Поскольку это абстрактный класс, думаю, что не стоит прописывать здесь какой-то смысловой код.

JuliGen · 2024-03-08T14:58:14Z

ELMOD1.py

+        Returns:
+        - RNASequence: Transcribed RNA sequence.
+        """
+        self.validate_alphabet()


В 3 предыдущих функция также происходит проверка на валидность алфавита. Наверное будет лучше 1 раз проверить алфавит при инициализации экземпляра, чтобы потом уже не проверять.

Suggested change

self.validate_alphabet()

JuliGen · 2024-03-08T15:01:39Z

ELMOD1.py

+            return 'A'
+        elif base == 'C':
+            return 'G'
+        elif base == 'G':


Если реализовать это через словарь - будет более красиво

JuliGen · 2024-03-08T15:16:18Z

ELMOD1.py

+    def is_valid_alphabet(self) -> bool:
+        """
+        Check if the DNA sequence contains a valid alphabet.
+
+        Returns:
+        - bool: True if the alphabet is valid, False otherwise.
+        """
+        return set(self.sequence) <= {'A', 'T', 'C', 'G'}
+
+class RNASequence(NucleicAcidSequence):
+    """
+    Class representing RNA sequences.
+    """
+    def is_valid_alphabet(self) -> bool:
+        """
+        Check if the RNA sequence contains a valid alphabet.
+
+        Returns:
+        - bool: True if the alphabet is valid, False otherwise.
+        """
+        return set(self.sequence) <= {'A', 'U', 'C', 'G'}


Для того, чтобы не прописывать в каждом классе, который наследуется от NucleicAcidSequence метод is_valid_alphabet можно сделать классовый атрибут alphabet для RNASequence и для DNASequence. И метод для проверки прописать в NucleicAcidSequence:

def is_valid_alphabet(self, seq: str) -> bool: """Checks the sequence to match the alphabet""" return set(seq).issubset(type(self).alphabet)

В данном случае в качестве alphabet возьмется алфавит в зависимости от класса.

У маркдауновского кода добавляйте python в первой строчке после ``` - так будет подсвечиваться синтакс:)

iliapopov17

Что ж мне всё очень понравилось! Всё довольно хорошо, вразумительно и толково. Мой код наверняка хуже, но я на него пока не хочу смотреть. Единственные два момента, которые, действительно, нуждаются в улучшении:

Комплементарность через словарь.
Валидация 3 раза подряд не нужна
Всё остальное это просто пожелания, чтобы из конфетки Ротфронт сделать конфетку Крокант.

iliapopov17 · 2024-03-10T10:01:59Z

ELMOD1.py

+    def brutto_count(self) -> List[Dict[str, int]]:
+        """
+        Calculate the Brutto formula values for each amino acid in the sequence.
+
+        Returns:
+        - List[Dict[str, int]]: List of dictionaries containing Brutto formula values for each amino acid.
+        """
+        self.validate_alphabet()
+        elements = ["C", "H", "N", "O", "S"]
+        result = []
+        for letter in self.sequence:
+            brutto_values = self.amino_brutto.get(letter, (0, 0, 0, 0, 0))
+            result.append(dict(zip(elements, brutto_values)))
+        return result


Вместо использования цикла for и append, можно использовать генератор списков для более компактного кода

Suggested change

def brutto_count(self) -> List[Dict[str, int]]:

"""

Calculate the Brutto formula values for each amino acid in the sequence.

Returns:

- List[Dict[str, int]]: List of dictionaries containing Brutto formula values for each amino acid.

"""

self.validate_alphabet()

elements = ["C", "H", "N", "O", "S"]

result = []

for letter in self.sequence:

brutto_values = self.amino_brutto.get(letter, (0, 0, 0, 0, 0))

result.append(dict(zip(elements, brutto_values)))

return result

def brutto_count(self) -> List[Dict[str, int]]:

"""

Calculate the Brutto formula values for each amino acid in the sequence.

Returns:

- List[Dict[str, int]]: List of dictionaries containing Brutto formula values for each amino acid.

"""

self.validate_alphabet()

elements = ["C", "H", "N", "O", "S"]

return [dict(zip(elements, self.amino_brutto.get(letter, (0, 0, 0, 0, 0)))) for letter in self.sequence]

Хорошее предложение! Хотя в таком случае надо следить за длиной строки

iliapopov17 · 2024-03-10T10:05:52Z

ELMOD1.py

+            if gc_pass and length_pass and quality_pass:
+                filtered_records.append(record)


Вместо if ___ and ___ and ___: можно всё запихнуть в if all:

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

Suggested change

if gc_pass and length_pass and quality_pass:

filtered_records.append(record)

if all([

self.check_bounds(gc_content, self.gc_bounds),

self.check_bounds(length, self.length_bounds),

avg_quality >= self.quality_threshold

]):

filtered_records.append(record)

Отличное предложение!

iliapopov17 · 2024-03-10T10:09:03Z

ELMOD1.py

+        self.validate_alphabet()
+        gc_count = sum(1 for base in self.sequence if base in {'G', 'C'})
+        return gc_count / len(self)


Мне кажется, такая запись будет более понятна

Suggested change

self.validate_alphabet()

gc_count = sum(1 for base in self.sequence if base in {'G', 'C'})

return gc_count / len(self)

self.validate_alphabet()

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

return gc_count / len(self)

Но так получается подсчет за 2 прогона. В общем получается:

Вариант немного эффективнее

Вариант немного понятнее

Какой лучше я тут сказать не могу, ахах, на ваш вкус)

iliapopov17 · 2024-03-10T10:14:46Z

ELMOD1.py

+    def read_fastq(self) -> List[SeqRecord]:
+        """
+        Read FastQ file and return a list of SeqRecord objects.
+        """
+        records = list(SeqIO.parse(self.input_path, "fastq"))
+        return records


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

Suggested change

def read_fastq(self) -> List[SeqRecord]:

"""

Read FastQ file and return a list of SeqRecord objects.

"""

records = list(SeqIO.parse(self.input_path, "fastq"))

return records

def read_fastq(self) -> List[SeqRecord]:

"""

Read FastQ file and return a list of SeqRecord objects.

"""

try:

records = list(SeqIO.parse(self.input_path, "fastq"))

return records

except IOError as e:

print(f"Error reading FastQ file: {e}")

return []

iliapopov17 · 2024-03-10T10:16:29Z

ELMOD1.py

+        if base == 'A':
+            return 'T'
+        elif base == 'T':
+            return 'A'
+        elif base == 'C':
+            return 'G'
+        elif base == 'G':
+            return 'C'


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

А что с другим ревьюером? Там вроде тоже советовали убрать это в словарь и совершенно верно, правильно что вы это написали

iliapopov17 · 2024-03-10T10:18:29Z

ELMOD1.py

+        SeqIO.write(records, output_path, "fastq")
+        print(f"Filtered sequences written to {output_path}")
+
+class BiologicalSequence(ABC):


Для абстрактного класса он слишком не абстрактный как-то... В нём много функционала в общем!

iliapopov17 · 2024-03-10T10:19:41Z

ELMOD1.py

+    def apply_filters(self, records: List[SeqRecord]) -> List[SeqRecord]:
+        """
+        Filter SeqRecord objects based on specified criteria.
+
+        Parameters:
+        - records (List[SeqRecord]): List of SeqRecord objects to filter.
+
+        Returns:
+        - List[SeqRecord]: Filtered list of SeqRecord objects.
+        """
+        filtered_records = []
+
+        for record in records:
+            gc_content = gc_fraction(record.seq)
+            length = len(record.seq)
+            avg_quality = sum(record.letter_annotations["phred_quality"]) / len(record.letter_annotations["phred_quality"])
+
+            gc_pass = self.check_bounds(gc_content, self.gc_bounds)
+            length_pass = self.check_bounds(length, self.length_bounds)
+            quality_pass = avg_quality >= self.quality_threshold
+
+            if gc_pass and length_pass and quality_pass:
+                filtered_records.append(record)
+
+        return filtered_records


Вместо вызова self.check_bounds для каждого критерия, можно вычислить результаты сразу:

Suggested change

def apply_filters(self, records: List[SeqRecord]) -> List[SeqRecord]:

"""

Filter SeqRecord objects based on specified criteria.

Parameters:

- records (List[SeqRecord]): List of SeqRecord objects to filter.

Returns:

- List[SeqRecord]: Filtered list of SeqRecord objects.

"""

filtered_records = []

for record in records:

gc_content = gc_fraction(record.seq)

length = len(record.seq)

avg_quality = sum(record.letter_annotations["phred_quality"]) / len(record.letter_annotations["phred_quality"])

gc_pass = self.check_bounds(gc_content, self.gc_bounds)

length_pass = self.check_bounds(length, self.length_bounds)

quality_pass = avg_quality >= self.quality_threshold

if gc_pass and length_pass and quality_pass:

filtered_records.append(record)

return filtered_records

def apply_filters(self, records: List[SeqRecord]) -> List[SeqRecord]:

"""

Filter SeqRecord objects based on specified criteria.

Parameters:

- records (List[SeqRecord]): List of SeqRecord objects to filter.

Returns:

- List[SeqRecord]: Filtered list of SeqRecord objects.

"""

return [record for record in records if

self.check_bounds(gc_fraction(record.seq), self.gc_bounds)

and self.check_bounds(len(record.seq), self.length_bounds)

and sum(record.letter_annotations["phred_quality"]) / len(record.letter_annotations["phred_quality"]) >= self.quality_threshold]

iliapopov17 · 2024-03-10T10:22:02Z

ELMOD1.py

+        Returns:
+        - RNASequence: Transcribed RNA sequence.
+        """
+        self.validate_alphabet()


Присоединяюсь к другим ревьюерам.

Add ELMOD1.py

cdd143b

Alisa411 reviewed Mar 6, 2024

View reviewed changes

JuliGen reviewed Mar 8, 2024

View reviewed changes

iliapopov17 reviewed Mar 10, 2024

View reviewed changes

		def __init__(self, input_path: str, output_filename: str, gc_bounds: Union[int, Tuple[int, int]] = (0, 100),
		length_bounds: Union[int, Tuple[int, int]] = (0, 2**32), quality_threshold: int = 0):


	if not os.path.isfile(self.input_path):
	raise FileNotFoundError(f"Input file '{self.input_path}' not found.")

		if not self.is_valid_alphabet():
		raise ValueError("Invalid alphabet for {}: {}".format(self.__class__.__name__, str(self)))

	return set(self.sequence) <= {'A', 'R', 'N', 'D', 'V', 'H', 'G', 'Q', 'E', 'I', 'L', 'K', 'M',
	return set(self) <= set(self.amino_brutto.keys())

		if gc_pass and length_pass and quality_pass:
		filtered_records.append(record)

Conversation

nvaulin commented Feb 26, 2024

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

Choose a reason for hiding this comment

Uh oh!