Bioinf dev tools by VovaGrig · Pull Request #1 · VovaGrig/IB_Bioinf_tools

VovaGrig · 2023-10-08T19:14:21Z

No description provided.

…naries.py

…ionaries.py

…torein_tools.py

pavlovanadia · 2023-10-16T18:07:00Z

+from src import dna_rna_tools
+from src import fastq_filter
+from src import protein_tools


две пустые строки после соответствует PEP8, но тут происходит горизонтальный импорт - импортируются скрипты, но вот для dictionaries надо писать отдельно, где его искать
Как итог: на каждом импорте код падает, траблшутинг написала в основном комментарии

pavlovanadia · 2023-10-16T18:07:06Z

+    - gc_bounds (int | float | tuple[int | float] | list[int | float]): GC content thresholds
+    - length_bounds (int | tuple[int] | list[int]): read length thresholds
+    - quality_thresholds: read Phred-33 scaled quality thresholds
+


Стоит в досктринг добавить описание verbose

pavlovanadia · 2023-10-16T18:07:08Z

+    if isinstance(gc_bounds, int) or isinstance(gc_bounds, float):
+        gc_bounds = (0, gc_bounds)


хорошая проверка типа

pavlovanadia · 2023-10-16T18:07:09Z

+    Return:
+    - condition (bool): True - quality is above threshold, False - read is to be filtered
+    """
+    mean_quality = sum(ord(symbol) - 33 for symbol in seq_qual) / len(seq_qual)


Хороший расчет

pavlovanadia · 2023-10-16T18:18:14Z

Отличная работа!
Комментарии:

структура репозитория хорошая, все на своих местах
README супер
Есть ощущение, что код из основного скрипта не тестировался, потому что при попытке запуска он падает с ошибкой импорта. Это связано с параллельным импортом, актуально для всех трех скриптов, решается конструкцией
if __name__ == "__main__": import dictionaries else: from src import dictionaries
потому что основной скрипт не особо знает, где ему искать словари. Следствие этого - ни одна из функций при вызове из основного скрипта не работает
значение fastq-файла по умолчанию ненужное и засоряет код, не надо так. Зачем пользователю при вызове функции без подачи аргумента с ридами получать вывод?
в конце кажджой строки докстринга \n лучше все же убирать, они не влияют на перенос строк
комментарии к прошлым дз учтены
Здорово, что был предусмотрен аргумент verbose для большей информатиновсти! Я бы, правда, предложила его сделать False по умолчанию, то есть не предлагать пользователю постоянно активно отказываться от вывода на экран, а наоборот, если он захочет больше информации, то активно на это согласиться :)

Итог:

За каждую из 3-x фильтраций fastq - по 1 баллу.
За главную функцию fastq-фильтратора - 1 балл.
За README - 2 балла
За улучшения кода ДНК/РНК и белковых тулов - 1 балл
За структуру репозитория и качество кода - 2.5 балла (-0.5 балла, т.к. горизонтальный импорт)

Бонусы и штрафы:

+0.1 за хорошую проверку типа границ
+0.1 за ord
-0.1 за дефолтное значение fastq, написанное прямо в аргументах функции. в целом такое значение по умолчанию делать в принципе не надо, но если хочется пользователю предложить что-то для запуска программы, то можно предоставить отдельный файл с тестовыми ридами, например, и в README написать команду для тестового запуска (если что, в следующих дз от вас это не требуется и не ожидается)

Сумма: 9.6

VovaGrig · 2023-10-16T18:34:38Z

Отличная работа! Комментарии:

структура репозитория хорошая, все на своих местах

README супер

Есть ощущение, что код из основного скрипта не тестировался, потому что при попытке запуска он падает с ошибкой импорта. Это связано с параллельным импортом, актуально для всех трех скриптов, решается конструкцией
if __name__ == "__main__": import dictionaries else: from src import dictionaries
потому что основной скрипт не особо знает, где ему искать словари. Следствие этого - ни одна из функций при вызове из основного скрипта не работает

значение fastq-файла по умолчанию ненужное и засоряет код, не надо так. Зачем пользователю при вызове функции без подачи аргумента с ридами получать вывод?

в конце кажджой строки докстринга \n лучше все же убирать, они не влияют на перенос строк

комментарии к прошлым дз учтены

Здорово, что был предусмотрен аргумент verbose для большей информатиновсти! Я бы, правда, предложила его сделать False по умолчанию, то есть не предлагать пользователю постоянно активно отказываться от вывода на экран, а наоборот, если он захочет больше информации, то активно на это согласиться :)

Итог:

За каждую из 3-x фильтраций fastq - по 1 баллу.

За главную функцию fastq-фильтратора - 1 балл.

За README - 2 балла

За улучшения кода ДНК/РНК и белковых тулов - 1 балл

За структуру репозитория и качество кода - 2.5 балла (-0.5 балла, т.к. горизонтальный импорт)

Бонусы и штрафы:

+0.1 за хорошую проверку типа границ

+0.1 за ord

-0.1 за дефолтное значение fastq, написанное прямо в аргументах функции. в целом такое значение по умолчанию делать в принципе не надо, но если хочется пользователю предложить что-то для запуска программы, то можно предоставить отдельный файл с тестовыми ридами, например, и в README написать команду для тестового запуска (если что, в следующих дз от вас это не требуется и не ожидается)

Сумма: 9.6

Код тестировался, моими кривыми руками) Я вручную добавлял пути в sys.path, т.к думал что проблема в использовании VS code и расширения wsl
Спасибо за комментарии и траблшутинг🔥 🔥

pavlovanadia · 2023-10-16T18:38:05Z

Если траблшутинг заключается в изменении переменной PATH, то это стоит написать в Installation and Usage :)
Но глобально - не надо (на практикуме есть некоторые тулы, с которыми так придется покопаться, и это не самая юзер-френдли история), если есть возможность написать код так, чтобы все работало сразу без изменения PATH

VovaGrig added 14 commits October 7, 2023 17:57

Create bioinf_tools.py

0ebdc00

Create bioinf_modules with dna_rna_tools.py, protein_tools.py, dictio…

bb282ed

…naries.py

Increase performance,add dockstrings to dna_rna_tools.py, modify dict…

a9106af

…ionaries.py

Increase performance, delete lower case in all dictionaries, update p…

eb189cb

…torein_tools.py

Create fastq_tools.py, functions and dockstrings within

0acfa30

Rename bioinf_modules to src

7671daa

Increase performance, update bioinf_tools.py

cd6bdcc

Rename fastq_tools.py to fastq_filter.py, update bioinf_tools.py

9edb943

Update README.md

3e68daa

Update README.md

c84a045

Update README.md

34b1e88

Update README.md

c2cac33

Update README.md

4b614be

Delete useless files

f243add

pavlovanadia reviewed Oct 16, 2023

View reviewed changes

Add fixes from PR

62bb4b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bioinf dev tools#1

Bioinf dev tools#1
VovaGrig wants to merge 15 commits intomainfrom
bioinf_dev_tools

VovaGrig commented Oct 8, 2023

Uh oh!

pavlovanadia Oct 16, 2023

Uh oh!

pavlovanadia Oct 16, 2023

Uh oh!

pavlovanadia Oct 16, 2023

Uh oh!

pavlovanadia Oct 16, 2023

Uh oh!

pavlovanadia commented Oct 16, 2023

Uh oh!

VovaGrig commented Oct 16, 2023

Uh oh!

pavlovanadia commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if isinstance(gc_bounds, int) or isinstance(gc_bounds, float):
		gc_bounds = (0, gc_bounds)

Conversation

VovaGrig commented Oct 8, 2023

Uh oh!

pavlovanadia Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

pavlovanadia Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

pavlovanadia Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

pavlovanadia Oct 16, 2023

Choose a reason for hiding this comment

Uh oh!

pavlovanadia commented Oct 16, 2023

Uh oh!

VovaGrig commented Oct 16, 2023

Uh oh!

pavlovanadia commented Oct 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants