Homework 5#1
Conversation
There was a problem hiding this comment.
Привет!
Хорошая работа
- Классный README, спасибо
- Неплохая структура коммитов:)
- Очень крутые изменения по прошлым ДЗ. На самом деле, не совсем все исправлено, но то что исправлено - оно прям основательно. Только смущает что одна из белковых функций так и не работает - отметил в коде.
- По фильтратору - отличная работа. Мне понравилось что как ты структурировал модуль. С отдельными проверками, отдельными местами для хранения констант. Логика довольно хорошо разбита. Были некоторые комментарии. Из общих и самых частны выделю пожалуй следующее:
- Константы называют капсом
- Проверки типов делают функцией
isinstance
У тебя оно конечно все работает и ок, но так просто принято.
- Много мелких комментов по качеству кода, не везде уже их отмечал. Будь чуть аккуратнее с такими вещами как нейминги аннотации типов, следи за пробелами и пустыми строками. В этом плане любая IDE в помощь. Оно не критично, но тут вот чуть-чуть не хватило чтобы совсем идеально было.
Баллы
- За модуль и качество кода 2.9/3
- За работу над ДЗ 3 и 4 0.9/1
Функция падает с ошибкой::(( Но объем исправлений на самом деле поражает - README 2/2
- За главную функцию фильтрации 1/1
- За 3 фильтратора 3/3
Итого: 9.8
Виден очень большой прогресс, ты молодец! Продолжаем в том же духе 💪🏻
There was a problem hiding this comment.
Очень основательное ридми, круто! Отдельный лайк за секцию Contacts and acknowledgements.
| return | ||
| - no return | ||
| """ | ||
| from modules.dna_rna_constants import dna_rna_alphabet |
There was a problem hiding this comment.
Импорты все таки лучше делать в начале скрипта вне функций. А то так каждый раз при вызове функции будет повторяться импорт
| """ | ||
| from modules.dna_rna_constants import dna_rna_alphabet | ||
| for seq in seqs: | ||
| if type(seq) != str: |
There was a problem hiding this comment.
Чаще принято все таки делать проверку через if isinstance(seq, str), хотя так тоже работает
There was a problem hiding this comment.
Ты же кстати вроде в ДЗ 3 и использовал её уже
| if i == 'T' or i == 't': | ||
| t_present = True | ||
| if i == 'U' or i == 'u': | ||
| u_present = True | ||
| if t_present and u_present: |
There was a problem hiding this comment.
В целом валидное решение с двумя флагами. Я лично чуть меньше люблю флаги, потому что за ними надо следить (а код иногда бывает очень большой). Но тут кажется это ок.
| raise ValueError('Invalid input: all sequences must be of type str!') | ||
| t_present = False | ||
| u_present = False | ||
| for i in seq: |
There was a problem hiding this comment.
Тут все таки лучше не i, это же не счетчик какой-то или индекс, это нуклеотид.
| for i in seq: | |
| for nucl in seq: |
| for i in seqs[seq_name][0]: | ||
| if i not in fastq_dna_code: |
There was a problem hiding this comment.
Тут лучше было бы сделать проверку на множествах:)
| @@ -0,0 +1,87 @@ | |||
| def check_fastq(seqs: dict): | |||
There was a problem hiding this comment.
Очень хорошая глубокая система проверок:)
| for i in seqs[seq_name][0]: | ||
| if i not in fastq_dna_code: | ||
| raise ValueError('Invalid input: sequences must contain only letters "A", "T", "G", "C" in upper case!') | ||
| if seq_name[0] != '@': |
There was a problem hiding this comment.
Мб кстати тут лучше было бы не [0] а seq_name.startswith('@'). По сути одно и то же, но читается имхо сильно понятнее
| for nucleotide in seq: | ||
| if nucleotide == 'G' or nucleotide == 'C': | ||
| gc_sum += 1 | ||
| return gc_bounds[0] <= gc_sum/len(seq)*100 <= gc_bounds[1] |
There was a problem hiding this comment.
Хех, компактность огонь:)
Мб деление на длину и умножение на 100 стоило бы даже на отдельную строчку вынести, все таки отдельный шаг логики
| return: | ||
| - (bool): whether length of a sequence is in the range provided | ||
| """ | ||
| return length_bounds[0] <= len(seq) <= length_bounds[1] |
2 participants
No description provided.