Review AGTRAP #29

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

Review AGTRAP

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

AGTRAP.py

+        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

AGTRAP.py

+    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

AGTRAP.py

+    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

AGTRAP.py

+    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

AGTRAP.py

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


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

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

AGTRAP.py

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


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

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

AGTRAP.py

+        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

AGTRAP.py

+    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

AGTRAP.py

+        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

AGTRAP.py

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


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

Suggested change

super().__init__()

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

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

AGTRAP.py

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


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

CaptnClementine

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

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

AGTRAP.py

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


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

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

AGTRAP.py

+        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

AGTRAP.py

+    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

AGTRAP.py

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


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

VovaGrig

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

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

AGTRAP.py

+    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

AGTRAP.py

+    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

AGTRAP.py

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


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

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

AGTRAP.py

+    @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

AGTRAP.py

+    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

AGTRAP.py

+    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

AGTRAP.py

+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

AGTRAP.py

+        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

AGTRAP.py

+            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

AGTRAP.py

+    :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

AGTRAP.py

+        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

AGTRAP.py

+    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

AGTRAP.py

+                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

AGTRAP.py

+            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

AGTRAP.py

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


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

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

AGTRAP.py

+        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

AGTRAP.py

+        '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

AGTRAP.py

+        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"):

Review AGTRAP #29

Are you sure you want to change the base?

Review AGTRAP #29

Uh oh!

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