Hw6 Beskrovnaia#2
Conversation
nvaulin
left a comment
nvaulin
left a comment
There was a problem hiding this comment.
Привет!
Че то на меня вдохновение нашло, добавил комменты и по прошлым домашкам. Общие моменты:
- Хорошая структура коммитов
- Мне понравился твой ридми, там только чуть чуть можно довести до идеала. И еще кстати заголовки как то появнее раздели. а то много жирного текста, местами теряешься в структуре
- По FASTQ в целом все ок, работает, есть только замечания по коду - обрати внимание.
- Конвертер фасты работает не правильно, к сожалению. Там не так много исправить чтобы сделать его правильным, но все же :(
- Вижу нету больше ничего. Это не страшно, правильно что сдала это! Можешь потом остальное доделать на /2 баллов.
Баллы
- Добработка FASTQ-модуля: 2 балла
- convert_multiline_fasta_to_oneline: 3/4 балла
- select_genes_from_gbk_to_fasta: 0/4 балла
-0.5 за общее качество кода: обманывающие нейминги, отсутсвие докстриги
Также вижу целых 2 коммита после дедлайна, поэтому, к сожалению, пришлось посмотреть что там: один импорт и редактирование содержания - это ок, ничего страшного, но на будущее мы следим за вами
Итого: 4.5 балла
Если что нехватка времени это ок. Код у тебя хороший - это главное! Потом когда будет минутка свободное все что надо добьешь.
| ## Table of Contents | ||
|
|
||
| - [Installation](#installation) | ||
| - [Functions](#functions) | ||
| - [Biotools.py](#biotools) | ||
| - [protein_tool](#protein_tool) | ||
| - [dna_rna_tools](#dna_rna_tools) | ||
| - [fastqc_filter](#fastqc_filter) | ||
| - [Bio_files_processor.py](#bio_files_processor) | ||
| - [Convert_multiline_fasta_to_oneline](#convert_multiline_fasta_to_oneline) | ||
|
|
|
|
||
| ## Installation | ||
|
|
||
| You can clone this repository or download the source code. |
There was a problem hiding this comment.
Здесь еще было бы хорошо в таком случае конкретно команду привести, чтобы человек мог прямо скопировать и не думать сам ни о чем
| ``` | ||
| ##### Example with custom quality_filter | ||
| ```python | ||
| fastqc_filter(input_path='f/fastq.txt', quality_threshold=(34)) |
There was a problem hiding this comment.
Тут подразумевается одно число подавать просто вот так:
| fastqc_filter(input_path='f/fastq.txt', quality_threshold=(34)) | |
| fastqc_filter(input_path='f/fastq.txt', quality_threshold=34) |
|
|
||
|
|
||
|
|
||
| def seq_transcribe(seq: str) -> str: |
There was a problem hiding this comment.
Не обязательно добавлять тут везде seq в названиях функции. Иногда даже линтеры ругаются если имя функции начинается или кончается именем какого-то из аргументов.
| def is_dna_or_rna(seq: str) -> bool: | ||
| unique_char_seq = set(seq) | ||
| return unique_char_seq <= NUC_FOR_DNA or unique_char_seq <= NUC_FOR_RNA No newline at end of file |
There was a problem hiding this comment.
Я бы лично разбил на две разные функции, is_dna и is_dna и использовал уже потом в условиях:
if is_dna(seq) or is_rna(seq):
...И можно было бы и по-отдельности юзать. Но ладно, это я так
|
|
||
|
|
||
|
|
||
| def fastqc_filter(input_path: str, output_filename: None = None, |
There was a problem hiding this comment.
| def fastqc_filter(input_path: str, output_filename: None = None, | |
| def fastq_filter(input_path: str, output_filename: None = None, |
Ниже прокомментировал это
| @@ -0,0 +1,21 @@ | |||
| import os | |||
|
|
|||
| def convert_multiline_fasta_to_oneline(input_fasta: str, output_fasta: str = None) -> None: | |||
There was a problem hiding this comment.
| def convert_multiline_fasta_to_oneline(input_fasta: str, output_fasta: str = None) -> None: | |
| def convert_multiline_fasta_to_oneline(input_fasta: str, output_fasta: Optiona[str] = None) -> None: |
There was a problem hiding this comment.
Спасибо за комментарии, но я не очень поняла, почему нет докстринги. В прошлых ревью мне писали, что все гуд. Или тут речь только конкретно про то, что надо было писать вот это optiona где None был
There was a problem hiding this comment.
Я имею ввиду тут у функций нету:
def func():
"""
Dosctring
"""
...Они у нас теперь к сожалению до самого конца ИБ должны быть :)
Если кто-то из ревьюеров случайно не обратил на это внимание - это скорее разовое везение чем правило
| output_fasta = os.path.basename(input_fasta) | ||
| output_path = os.path.join(out_dir, output_fasta) | ||
| else: | ||
| output_filename = output_fasta + '.fasta' |
There was a problem hiding this comment.
В целом ок, но я бы проверил наличие расширения - мало ли что
| output_path = os.path.join(out_dir, output_fasta) | ||
| else: | ||
| output_filename = output_fasta + '.fasta' | ||
| output_path = os.path.join(out_dir, output_filename) |
There was a problem hiding this comment.
Это можно вынести из условий. По сути условия это выбор имени файла. А тут ты конструируешь путь, и это уже не зависит от того какое у нас там имя файла.
| with open(os.path.join(output_path), mode = 'w') as output_file: | ||
| output_file.write(oneline_seqs) |
There was a problem hiding this comment.
Какая-то фаста получилась неудачная:)
Ахахаха
- В fasta всегда есть имя - это строка с >
- Ты склеила со всем все строки в один - так не надо. Если ты посмотришь файл-пример, то там каждая запись - это отдельный тип РНК. Надо было в рамках каждой записи собрать все строки в одну и при этом оставить названия записей.
2 participants
No description provided.