-
Notifications
You must be signed in to change notification settings - Fork 45
Hw4 bobkov #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Hw4 bobkov #8
Conversation
Hw4 bobkov
Hw4 bobkov
Add verify function and is_protein and strink_check subfunctions
Picture with English file name
Correct minor spelling and wording errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет,
❗️ Напоминание напомнить всем членам команды посмотреть фидбек.
Неплохая работа, общие комментарии:
-
Хорошие названия коммитов, хотя в какой то момент времени были прболемки. Например, это что?:) folder
-
Очень хороший README по содержанию. Прям как надо. 2 момента
- Чуть у вас немного сбивают с толку большое количество похожих заголовков. Где как будто бы весь текст это только заголовки
- Не все примеры из README работают)) Это плохо
-
По коду в целом нормально. Есть некоторые проблемки
- Нейминги, пустые строки, пробелы. Где то я обращал на это внимание, но не везде. Проверьте это каким нибудь линтером автоматическим.
- Словари со всякой инфой про белки лучше было бы вынести в константы. Если что см. конслуьтацию 03.10.23
- Где то у вас функции совсем простого или однотипного содержания. А еще для сравнения строк можно их просто сравинать через ==.
- Было бы здорово поддерживать и 1буквенный и 3буквкенный ввод. Все таки зачастую люди помнят именно 3буквенные обозначения.
- Формат вывода:
- В разных функциях разный. Это не хорошо. Где то список, где то словарь. Лучше одинаково чтобы было
- Оптимально мне кажется было бы выводить словарем, где ключ - это введенное значение, а значение словаря - это результат. Который может быть строкой, числом, словарем - что нужно. Да, даже если это будет словарь из словарей.
- По вводу тоже у вас небольшая путаница. Все таки лучше всякие специальные вещи типа паттерна и т п принимать не в числе позиционных и вычлинять оттуда, а сразу именованно
- У вас были места где вы итерируетесь по индексам, хотя можно было бы сразу по элементам
- Были некоторые проблемы с логикой кое-где (очень похожий функционал повторяется, функции и словарем и if-else, избыточное итерирование по индексам, какие-то мудреные вложенные конструкции которые не всегда очевидно читаются)
Баллы:
- README 2.4/2.5
- Операции:
- compare 0.5/1.5, не удалось запустить как в примере в README
- pattern 0.5/1.5, не удалось запустить как в примере в README
- остальные 1.5*3 = 4.5/4.5
Кое-где нет аннотации типов: -0.0001
Проблемы с неймингами: -0.5
Проблемы с проблеами и пустыми строками: -0.2
За логику -0.3
+0.2 за то что глубоко проработаны ошибки, но там есть что еще улучшить)
+0.2 за фото команды в README
Итого: 7.3 балла
### :warning: Attention: | ||
### 1)> The programm is register-dependent. | ||
### 2)> Before using some of the options read 'Procedures description' carefully. | ||
### 3)> If you input sequenses or 'options' incorrectly, the program will provide you with helpful error messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кажется тут немного перебор с оформлением)
@@ -0,0 +1,120 @@ | |||
# protein_tools.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Классное README!
|
||
**list of options:** | ||
|
||
- 'compare' - Compare amino acids between reference sequence and other sequences; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут можно было бы сделать не одинарные кавычки, а штрихи (на букве Ё) чтобы оформить эти опции как код.
- 'compare' - Compare amino acids between reference sequence and other sequences; | |
- `compare` - Compare amino acids between reference sequence and other sequences; |
# Procedures description | ||
## compare | ||
### Introduction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Понимаю что вы шли просто последовательно по заголовкам разного уровня. Но итогово опять же выглядит немного перегруженно.
## compare | ||
### Introduction | ||
The **compare** procedure compares the first amino acid sequence provided with the following ones. | ||
### Inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы эти штуки оформил не как заголовки, а просто жирным.
По в любом случае структура хорошая
|
||
def protein_tool(*proteins, options = None): | ||
proteins = list(proteins) | ||
verify(proteins, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Очень круто что вы сделали такой чекер ввода. Это хорошо.
Но я пока читал его совсем в нем потерялся. if внутри elif внутри for внутри elif. Жуть. Может попробовать как то поработать над структурой. Например вижу что вы можете кое-где итерироваться не по индексам а сразу по элементам. Это уже сильно упростит читаемость.
Но с другой стороны хорошо что прям такие ошибки подробные расписаны. Любой пользователь был бы за это благодарен.
operations = { | ||
'compare': compare, | ||
'length': count_length, | ||
'percentage': count_percentage, | ||
'pattern': find_pattern, | ||
'3Letter_name': rename_three_letter_name, | ||
'DNA_code': transform_to_DNA_code | ||
} | ||
|
||
if options == 'compare': | ||
result = operations[options](proteins[:-2], proteins[-2], proteins[-1]) | ||
return (result) | ||
elif options == 'pattern': | ||
result = operations[options](proteins[1:len(proteins)],proteins[0]) | ||
return (result) | ||
elif options == '3Letter_name': | ||
result = operations[options](proteins[:-1], proteins[-1]) | ||
return (result) | ||
elif options == 'length' or options =='percentage' or options == 'DNA_code': | ||
result = [] | ||
for protein in proteins: | ||
res = operations[options](protein) | ||
result.append(res) | ||
return (result) | ||
else: | ||
raise ValueError('Incorrect options input, please try again') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ахах, ну не, смотрите. У всех методов есть область применимости. Кое где словари хороши, кое где нет. У вас тут у каждой функции свой набор аргументов. Поэтому получается вообще нет смысла писать через словари. Просто if-else и там вызываете нужную функцию.
Либо словари либо if-else. Тут очевидно нужен второй вариант.
else: | ||
raise ValueError('Incorrect options input, please try again') | ||
|
||
protein_tool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не надо было эту функцию тут вызывать. Тем более зачем? Она же без аргументов и не работает
result.append(res) | ||
return (result) | ||
else: | ||
raise ValueError('Incorrect options input, please try again') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я бы это вынес в начало тоже в проверку аргументов, что опция есть в списке опций. Чтобы разнести логику проверок и работы.
```python | ||
protein_tool('wWGPdPA', '', options = '3Letter_name') # ['trpTRPGLYPROaspPROALA'] | ||
protein_tool('LAlLAlwWGPdPA', '-', options = '3Letter_name') # ['LEU-ALA-leu-LEU-ALA-leu-trp-TRP-GLY-PRO-asp-PRO-ALA'] | ||
protein_tool('RRRrrrR', 'WGPdPA', 'LAlLAlw', options = 'percentage') # [{'R': 57.14, 'r': 42.86}, {'P': 33.33, 'W': 16.67, 'G': 16.67, 'd': 16.67, 'A': 16.67}, {'L': 28.57, 'A': 28.57, 'l': 28.57, 'w': 14.29}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
этого тут не должно быть вроде
No description provided.