Skip to content

Feedback#1

Open
ArtemVaska wants to merge 27 commits intomainfrom
dev
Open

Feedback#1
ArtemVaska wants to merge 27 commits intomainfrom
dev

Conversation

@ArtemVaska
Copy link
Owner

No description provided.

ArtemVaska and others added 27 commits October 7, 2023 16:11
Add dna_tools.py, protein_tools.py, fastq_tools.py
Add "Possible commands" to run_dna_rna_tools and run_ultimate_protein_tools functions
Copy link

@albidgy albidgy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Общие комментарии:

  • Работа правда хорошая! Сделана аккуратно. Я открывала ее в PyCharm, и он практически ни на что не ругался (в плане стиля кода).
  • В README не хватает примеров для функций dna_rna_tools и protein_tools. А вот часть про фильтрацию fastq - отличная.
  • Комментарии к коммитам замечательные!
  • Обрати внимание на то, как иначе можно было сделать проверку качества ридов.
  • Постарайся чуть более пристально писать Typing, в некоторых функциях встречается, что не все возможные типы указаны.
  • Большинство замечаний учтено, но часть комментариев Никиты по обработке аминокислотных последовательностей не учтена.

Баллы:

  • 3 фильтрации FASTQ 3/3
  • Главная функция 1/1
    README 1.8/2 (-0.2 за отсутствие примеров)
    Структура репозитория и качество кода 2.9/3 (-0.1 за Typing)
    Улучшение кода ДНК/РНК и белковых тулов 0.8/1

Итого: 9.5 баллов

Comment on lines +10 to +12
```python
import ultimate_tools as ut
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тогда уже можно было бы написать, что нужно скачать проект с гитхаба git clone... Но это нюанс.

@@ -0,0 +1,167 @@
from typing import List, Tuple, Dict

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Лучше ставить 2 переноса строки. По PEP8 можно и так, и так, но чаще встречается все же вариант с переносом 2 строк

'I': [73, 40]
}

EXAMPLE_FASTQ = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не надо вставлять пример прям в код, смотрится нагроможденно, лучше вынести в README

- float: GC-content in percentages
"""

return (seq.upper().count('G') + seq.upper().count('C')) / len(seq) * 100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здорово, что учел регистр

Comment on lines +157 to +167
for key in seqs:
qual_seq = seqs[key][1]
qual_list = []
for value in qual_seq:
qual_list.append(ASCII_Q_SCORE[value][1])
mean_qual = sum(qual_list) / len(qual_list)
if mean_qual >= quality_threshold:
quality_in_bounds[key] = True
else:
quality_in_bounds[key] = False
return quality_in_bounds
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот это решение верное, но неудачное. Лучше было не заводить словарь, а погуглить, как это можно автоматизировать. Логика PHRED базируется на том, что качество представляется в виде конкретного символа, значение которого можно получить, сделав перевод в Юникод. Тогда логику функции quality_seq можно сделать так:

def mean_quality_seq(qual_seq):
    sum_phred = 0
    for elem in qual_seq:
        sum_phred += ord(elem) - 33
    mean_quality = sum_phred / len(qual_seq)
    return mean_quality

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

- Dict[str, Tuple[str]]: a dictionary with filtered sequences
"""

if isinstance(gc_bounds, (int, float)): # input check
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кстати, забавный факт, с python3.10 можно еще и так писать:

Suggested change
if isinstance(gc_bounds, (int, float)): # input check
if isinstance(gc_bounds, int | float): # input check

Comment on lines +34 to +38
if gc_bounds[1] > 100 or \
(gc_bounds[0] > gc_bounds[1]) or \
(length_bounds[0] > length_bounds[1]) or \
not 0 <= quality_threshold <= 40: # bounds check
raise ValueError('The bounds are indicated incorrectly!')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здорово, что сделал проверку на корректность входных данных

Comment on lines +41 to +42
quality_in_bounds = (
fastq_tools.is_quality_in_bounds(seqs, quality_threshold))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не поняла, зачем здесь внешние скобки, без них работает также

Suggested change
quality_in_bounds = (
fastq_tools.is_quality_in_bounds(seqs, quality_threshold))
quality_in_bounds = fastq_tools.is_quality_in_bounds(seqs, quality_threshold)

fastq_tools.is_quality_in_bounds(seqs, quality_threshold))

filtered_fastq_seqs = {}
for key in seqs:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вообще в этой функции несколько неоптимально идет проверка. Каждая из 3 функций-проверок иттерируются по словарю с ридами. Лучше цикл for вынести в тело главной функции. Тогда бы не нужны были списки, в которых хранятся True/False, а для каждой последовательности индивидуально принималось решение.

Comment on lines +46 to +48
if (seqs_gc_and_len_in_bounds[key][0] and
seqs_gc_and_len_in_bounds[key][1] and
quality_in_bounds[key]) is True:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Здесь не нужно писать is True. Фактически в условиях if проводится какая-то проверка, которая возвращает True/False. Если значение True, то тогда код заходит в тело if. Твои функции и так содержать болевые значения True/False. Например:

a = 2
if a == 2: -> True
   some action 

Поэтому твою запись можно сделать так:

Suggested change
if (seqs_gc_and_len_in_bounds[key][0] and
seqs_gc_and_len_in_bounds[key][1] and
quality_in_bounds[key]) is True:
if (seqs_gc_and_len_in_bounds[key][0] and
seqs_gc_and_len_in_bounds[key][1] and
quality_in_bounds[key]):

То есть это фактически

        if (True and True and True) == True:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants