Review AGTRAP by nvaulin · Pull Request #29 · Python-BI-2023/Peer_review

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

Review AGTRAP

CaptnClementine · 2024-03-06T15:50:27Z

+        output_filename = os.path.splitext(os.path.basename(input_path))[0]
+    output_filename = f'{output_filename}.fastq'
+
+    os.makedirs('fastq_filtrator_resuls', exist_ok=True)


класс, без лишних проверок - просто параметром проверка существования файла

CaptnClementine · 2024-03-06T16:01:54Z

+    count_record = 0
+
+    with open(os.path.join(output_dir, output_filename), mode='w') as out_file:
+        for record in SeqIO.parse("reads.3.fastq", "fastq"):


Кажется кто-то забыл тут свой файлик. Возможно, должно быть

Suggested change

for record in SeqIO.parse("reads.3.fastq", "fastq"):

for record in SeqIO.parse(input_path, "fastq"):

:)

Хе-хе, да

CaptnClementine · 2024-03-06T16:12:15Z

+    if report:
+        print(f' Number of source sequences = {count_record},'
+              f'\n Number of sequences after filtering = {count_filtered_record},')


крутая идея с report!

CaptnClementine · 2024-03-06T16:13:58Z

+    def __getitem__(self, key: Union[int, slice]) -> object:
+        """Returnы a subsequence in the form of a single letter or a string of several letters"""
+        if isinstance(key, int):
+            if 0 >= key or key > len(self.seq):
+                raise IndexError(f"Index {key} is out of range of the sequence")
+            return super().__getitem__(key - 1)
+        elif isinstance(key, slice):
+            if key.start <= 0 or key.stop > len(self.seq):
+                raise IndexError(f"Index {key} is out of range of the sequence")
+            new_slice = slice(key.start - 1, key.stop, key.step)
+            return super().__getitem__(new_slice)


Кажется, что в абстрактном классе определять функции все же не стоит. Все равно мы должны их переопределить в дочерних классах)

CaptnClementine · 2024-03-06T16:14:45Z

+        """Checks the sequence to match the alphabet"""
+        pass
+
+    def __str__(self):


Опять же, все функции абстрактного класса должны быть переопределены в дочерних. Лучше унести это вниз

CaptnClementine · 2024-03-06T16:15:50Z

+    def transcribe(self) -> str:
+        """ Returns the transcribed DNA """
+        return RNASequence(self.seq.replace('T', 'U').replace('t', 'u'))


Коротко, красиов и возращает то что нужно - класс!

CaptnClementine · 2024-03-06T16:17:58Z

+        elif record_type == "three_letter_sequence":
+            slice_sequence = [seq.upper()[i:i + 3] for i in range(0, len(seq), 3)]
+            return set(slice_sequence).issubset(type(self).three_letter_alphabet)


круто что есть возможность передавать в 3 буквенном коде!

CaptnClementine · 2024-03-06T16:20:28Z

+    alphabet = set('ATCGUatcgu')
+    compliment_dict = None


Интересно, зачем тут алфавит, если он переопределяется дочерними классами? Хотя алфавит, конечно, логичный)

Suggested change

alphabet = set('ATCGUatcgu')

compliment_dict = None

alphabet = None

compliment_dict = None

CaptnClementine · 2024-03-06T16:21:38Z

+        if type(self) is NucleicAcidSequence:
+            raise NotImplementedError("The compliment method cannot be called for an object of the NucleicAcidSequence")


Красиво!

CaptnClementine · 2024-03-06T16:24:45Z

+class BiologicalSequence(ABC, str):
+
+    def __init__(self, seq: str):
+        super().__init__()


не очень понимаю зачем здесь это, хочешь вызвать init от str? Зачем?

Suggested change

super().__init__()

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

CaptnClementine · 2024-03-06T16:25:10Z

+    def __init__(self, seq: str):
+        super().__init__()
+        self.seq = seq


В идеале в абстрактном классе init быть не должно

CaptnClementine

Это очень качественная работа, заставила узнать что я не знала и вспомнить многие декараторы - очень и очень круто! Спасибо! Взяла некоторые вещи себе на на заметку :)

CaptnClementine · 2024-03-06T16:40:37Z

+
+    def reverse(self):
+        """Create a reverse sequence"""
+        return self.__class__(self.seq[::-1])


Круто реализовано!

CaptnClementine · 2024-03-06T16:43:30Z

+        if self._check_alphabet(seq):
+            super().__init__(seq)
+        else:
+            raise ValueError(f"The sequence does not match the alphabet for the {self.__class__.__name__}")


CaptnClementine · 2024-03-06T16:54:04Z

+    def __getitem__(self, key: Union[int, slice]) -> object:
+        """Returns a subsequence in the form of a single letter or a string of several letters"""
+        return super().__getitem__(key)
+
+    def __len__(self) -> int:
+        """Return the length of the sequence"""
+        return super().__len__()


А я и не знала что так можно! Очень круто!!!

CaptnClementine · 2024-03-06T16:56:44Z

+            return super().__getitem__(key)
+
+        if self.record_type == "three_letter_sequence":
+            if isinstance(key, int):


Я уже не могу столько писать, что крутая работа, поэтом вот - еще один комплемент, но на целый блок)))

VovaGrig

Хорошая работа) Кажется, всё же от абстрактных классов наследовать через super не стоит, лучше переопределять их в дочерних классах) Докстринги подробные, аннотации типов местами не хватает, но код читается хорошо. Спасибо за работу!)

VovaGrig · 2024-03-10T22:05:56Z

+    if not isinstance(gc_bounds, tuple):
+        gc_bounds = (0, gc_bounds)
+    if not isinstance(length_bounds, tuple):
+        length_bounds = (0, length_bounds)


Кажется, хорошо бы проверить, что если эти параметры не тюплы, то числа. А то получается, что и string можно засунуть, если они не тюплы

VovaGrig · 2024-03-10T22:08:18Z

+    with open(os.path.join(output_dir, output_filename), mode='w') as out_file:
+        for record in SeqIO.parse("reads.3.fastq", "fastq"):


Тут ошибочка... Я так подозреваю reads.3.fastq на самом деле это input_path

Suggested change

with open(os.path.join(output_dir, output_filename), mode='w') as out_file:

for record in SeqIO.parse("reads.3.fastq", "fastq"):

with open(os.path.join(output_dir, output_filename), mode='w') as out_file:

for record in SeqIO.parse(input_path, "fastq"):

VovaGrig · 2024-03-10T22:10:41Z

+    def __init__(self, seq: str):
+        super().__init__()
+        self.seq = seq


Кажется init лишний для абстрактного класса)

VovaGrig · 2024-03-10T22:12:41Z

+    @abstractmethod
+    def __len__(self):
+        """Return the length of the sequence"""
+        return super().__len__()
+
+    @abstractmethod
+    def __getitem__(self, key: Union[int, slice]) -> object:
+        """Returnы a subsequence in the form of a single letter or a string of several letters"""
+        if isinstance(key, int):
+            if 0 >= key or key > len(self.seq):
+                raise IndexError(f"Index {key} is out of range of the sequence")
+            return super().__getitem__(key - 1)
+        elif isinstance(key, slice):
+            if key.start <= 0 or key.stop > len(self.seq):
+                raise IndexError(f"Index {key} is out of range of the sequence")
+            new_slice = slice(key.start - 1, key.stop, key.step)
+            return super().__getitem__(new_slice)


Функции позже в дочерних классах переопределяются, абстракный класс выступает в качестве шаблона, потому в нём подобные определения методов не нужны)

VovaGrig · 2024-03-10T22:14:33Z

+    alphabet = set('ATCGUatcgu')
+    compliment_dict = None


Такс, тут нужно бы определять через self , и нет нужды сохранять алфавит, если позже он будет переопределяться

Suggested change

alphabet = set('ATCGUatcgu')

compliment_dict = None

set.alphabet = None

set. compliment_dict = None

VovaGrig · 2024-03-10T22:16:51Z

+    def __init__(self, seq: str):
+        if self._check_alphabet(seq):
+            super().__init__(seq)
+        else:
+            raise ValueError(f"The sequence does not match the alphabet for the {self.__class__.__name__}")
+
+    def __getitem__(self, key: Union[int, slice]) -> object:
+        """Returns a subsequence in the form of a single letter or a string of several letters"""
+        return super().__getitem__(key)
+
+    def __len__(self) -> int:
+        """Return the length of the sequence"""
+        return super().__len__()


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

VovaGrig · 2024-03-10T22:21:59Z

+class AminoAcidSequence(BiologicalSequence):
+    one_letter_alphabet = set('GAVLIMPFWSTNQYCKRHDE')
+    three_letter_alphabet = {'GLY', 'ALA', 'VAL', 'LEU', 'ILE', 'MET', 'PRO', 'PHE', 'TRP', 'SER', 'THR', 'ASN', 'GLN',
+                             'TYR', 'CYS', 'LYS', 'ARG', 'HIS', 'ASP', 'GLU'
+                             }
+    amino_acid_weights = {
+        'A': 89, 'R': 174, 'N': 132, 'D': 133, 'C': 121,
+        'E': 147, 'Q': 146, 'G': 75, 'H': 155, 'I': 131,
+        'L': 131, 'K': 146, 'M': 149, 'F': 165, 'P': 115,
+        'S': 105, 'T': 119, 'W': 204, 'Y': 181, 'V': 117,
+        'GLY': 75, 'ALA': 89, 'VAL': 117, 'LEU': 131, 'ILE': 131, 'MET': 149,
+        'PRO': 115, 'PHE': 165, 'TRP': 204, 'SER': 105, 'THR': 119, 'ASN': 132, 'GLN': 146,
+        'TYR': 181, 'CYS': 121, 'LYS': 146, 'ARG': 174, 'HIS': 155, 'ASP': 133, 'GLU': 147,
+        }


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

VovaGrig · 2024-03-10T22:28:23Z

+        if type(self) is NucleicAcidSequence:
+            raise NotImplementedError("The compliment method cannot be called for an object of the NucleicAcidSequence")


Круто! То, что комплемент не может вызываться для NucleicAcidSequence кажется разумным, в прошлом ревью я это не заметил)))) А у тебя клёво)

VovaGrig · 2024-03-10T22:29:36Z

+            if isinstance(key, int):
+                if 0 >= key or key > len(self.seq):
+                    raise IndexError(f"Index {key} is out of range of the sequence")
+                return super().__getitem__(slice((key * 3) - 2, key * 3))


Чуть сложно воспринимается(

GlebBobkov · 2024-03-09T16:44:02Z

+    :param report:report = True (show filtering report), report = True (default)
+    :return:A filtered fastq file containing sequences
+             that have passed all tests (according to specified parameters)
+    """


Ну это чисто отвал фляги, спасибо за подбробную аннотацию, это невероятно приятно видеть и читать

GlebBobkov · 2024-03-09T17:09:31Z

+        output_filename = os.path.splitext(os.path.basename(input_path))[0]
+    output_filename = f'{output_filename}.fastq'
+
+    os.makedirs('fastq_filtrator_resuls', exist_ok=True)


не знал, что так можно. интересно и хорошо

GlebBobkov · 2024-03-09T17:16:55Z

+    count_filtered_record = 0
+    count_record = 0
+
+    with open(os.path.join(output_dir, output_filename), mode='w') as out_file:


вероятно тут просто input_path

GlebBobkov · 2024-03-09T17:20:41Z

+                if length_bounds[0] <= len(record.seq) <= length_bounds[1]:
+                    if np.mean(record.letter_annotations['phred_quality']) > quality_threshold:
+                        count_filtered_record += 1
+                        SeqIO.write(record, out_file, "fastq")


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

GlebBobkov · 2024-03-09T17:27:41Z

+            if key.start <= 0 or key.stop > len(self.seq):
+                raise IndexError(f"Index {key} is out of range of the sequence")
+            new_slice = slice(key.start - 1, key.stop, key.step)
+            return super().__getitem__(new_slice)


Все хорошо и понятно

GlebBobkov · 2024-03-09T17:29:42Z

+
+class NucleicAcidSequence(BiologicalSequence):
+    alphabet = set('ATCGUatcgu')
+    compliment_dict = None


Очень хороший словарь для проверки сразу двух аминокислот, мне кажется, что действительно правильно писать его в двух регистрах, потому что пользователь может писать в обоих форматах, это прям хорошо

GlebBobkov · 2024-03-09T17:29:54Z

+        if self._check_alphabet(seq):
+            super().__init__(seq)
+        else:
+            raise ValueError(f"The sequence does not match the alphabet for the {self.__class__.__name__}")


красиво!!!

GlebBobkov · 2024-03-09T17:31:51Z

+        'PRO': 115, 'PHE': 165, 'TRP': 204, 'SER': 105, 'THR': 119, 'ASN': 132, 'GLN': 146,
+        'TYR': 181, 'CYS': 121, 'LYS': 146, 'ARG': 174, 'HIS': 155, 'ASP': 133, 'GLU': 147,
+        }
+


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

GlebBobkov · 2024-03-09T17:32:34Z

+        elif self.record_type == "three_letter_sequence":
+            for i in range(0, len(self.seq), 3):
+                molecular_weight += type(self).amino_acid_weights[self.seq[i:i + 3].upper()]
+        return molecular_weight


Это очень хорошая и красивая работа, из которой я многое подчерпнул для себя, спасибо за такую работу, она прям очень хороша и красива

Add AGTRAP.py

481eed5

CaptnClementine reviewed Mar 6, 2024

View reviewed changes

VovaGrig reviewed Mar 10, 2024

View reviewed changes

GlebBobkov reviewed Apr 11, 2024

View reviewed changes

	for record in SeqIO.parse("reads.3.fastq", "fastq"):
	for record in SeqIO.parse(input_path, "fastq"):

		if type(self) is NucleicAcidSequence:
		raise NotImplementedError("The compliment method cannot be called for an object of the NucleicAcidSequence")

		with open(os.path.join(output_dir, output_filename), mode='w') as out_file:
		for record in SeqIO.parse("reads.3.fastq", "fastq"):

Conversation

nvaulin commented Feb 26, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

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