Skip to content

HW4_Sapozhnikov #22

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Conversation

NSapozhnikov
Copy link

No description provided.

NSapozhnikov and others added 30 commits September 26, 2023 07:14
Add check_input() to check the validity of the input in main()
Add local_alignment() to perform Smith-Waterman algorithm
Add prettify_alignment() to prettify the view of an alignment
PR local alignment functionality
@NSapozhnikov NSapozhnikov changed the title Stable HW4_Sapozhnikov Oct 1, 2023
Copy link
Member

@nvaulin nvaulin left a comment

Choose a reason for hiding this comment

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

Привет!

❗️ Напоминание напомнить всем членам команды посмотреть фидбек.

Классная работа! Общие моменты:

  1. Структура коммитов где то хорошая, а где то не очен.
    • Вот это хороший коммит Add local alignment function
    • Вот этот стоило бы разбить Add local alignment functionality
    • Некоторые коммиты называются с маленькой буквы
    • У этого коммита комментарий не соотвествует названию фунции (при чем я не вижу коммитов сообщение которых соотвествует переименованию). Add gc_content function
  2. Были небольшие проблемы со структурой репозитория. В данном случае .gitignore не надо было добавлять в git. Тем не менее хорошо что вы их пофиксили и очистили все ненужные файлы. Молодцы! (но в следующий раз сразу будьте аккуратнее)
  3. README хорошое. Здорово что описаны все примеры и опции использования. Ссылка на алгоритм это правильно. Тем не менее, можно было бы, например, не делать столько горизонтальных линий. Все таки у вас они в основном там, где по идее нужен просто bullet-list. Здорово что описали список авторов и зон отвественности.
  4. По коду - в основном хороший код.
    • Почти везде чистый, были пару моментов с плохими неймингами, лишними пустыми строками, пробелами и .т.д.
    • Отдельно хочу подчеркнуть понятие консистентности. Даже если можно сделать по-разному, в рамках одного проекта лучше stick to один какой-то способ.
    • Круто что сделали словари-константы. Хорошие докстринги, для внутренних функций я даже ожидал бы чуть менее подробные. В общем молодцы.
    • Есть кое-где какие-то неочевидные вещи, которые я старался прокомментировать.
    • С общим дизайном тоже все хорошо но не идеально. Так, у вас некоторые функции итерируются по белкам, а некоторые принимают только один белок. Где-то у вас повторяются проверки, хотя есть специальная функция для этого. Ну и функция эта кажется не только делает проверки, но и как-то обрабатывает ввод, что уже не очень хорошо. А еще какие то функции у вас ждут однобуквенный код, а какие-то трехбуквенный. Вот тут я совсем потрялся. Хорошо что они могут конвертировать в нужный себе формат, но опять же, почему не консистентно?

Комментарии по функциям:

  1. Функция recode возвращает ответ в виде словаря, но как-то странно.
    Там {3-letter code : 1-letter code}
    То есть запустив код main('MNT', method = 'recode') я бы ожидал получить MetAsnThr, или хотя бы {'MNT': 'MetAsnThr'} (запрос : ответ). А у вас получается наоборот. Я вашу логику понимаю, но я бы такого от тула никогда б не ожидал))

  2. main('MNT', 'MNT', method='local_alignment') дает ошибку (local variable 'seq_on' referenced before assignment). При чем кажется падает на чекере. Проверьте.
    И еще кажется я не могу подать однобуквенное выравнивание на вход алайнера. И еще когда я вызываю на двух одинаковых посдедовательностях, он мне почему-то выдает выравнивание лишь одной буквы))

main('AlaValTyrAlaValTyr', 'AlaValTyrAlaValTyr', method='local_alignment')
A
|
A
  1. Если я подаю на запрос, например, M, то я получаю ответ для Met. Тоже не хорошо. Я получается должен знать, что сделать запрос я могу в любой форме, но чтобы получить результат мне надо че то еще перекодировать ( в смысле чтобы достать ответ из словаря). В общем не очень хорошо. Я как попросил так и хочу получить ответ. Скорее всего у меня там же пайплайн какой то. Ну и + когда я отдаю один белок на вычисление массы, я бы сразу хотел получить число, а не словарь с числом. Как в домашке по ДНК РНК.

Баллы.

  • README 2.4/2.5
  • local_alignment 0.5/1.5 (круто, но не работает))
  • Остальные функции 4*1.3/1.5 = 5.2/6 (см. комменты по формату вывода)
  • За структуру коммитов -0.1
  • За проблемы с пустыми строками / обрывами строк / пробелами -0.2
  • За проблемы с неймингами кое-где -0.3
  • За небольшие проблемы с общим дизайном, логикой и консистентностью -0.3

За 2 другие поставлю по +0.4 т.к. они работают. В любом случае, в зачет идут только 5, это небольшой бонус, т.к. вы потеряли много на выравнивателе. С одной стороны, здорово что попробовали, с другой же лучше выдавать надежно готовый продукт.

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

### Overview
`prototool.py` includes 7 methods to treatment of polyaminoacid sequences.
`prototool.py` can be used for the next goals:
- recoding 1-letter coded polyaminoacid seqeunces into 3-letter coded and vice versa;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- recoding 1-letter coded polyaminoacid seqeunces into 3-letter coded and vice versa;
- recoding 1-letter coded polyaminoacid seqeunces into 3-letter coded and *vice versa*;

Comment on lines +15 to +16
- determining polyaminoacid isoelectric point;
- calculating polyaminoacid molecular weight;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- determining polyaminoacid isoelectric point;
- calculating polyaminoacid molecular weight;
- calculating polyaminoacid isoelectric point;
- calculating polyaminoacid molecular weight;


### Examples

def recode allows to translate 1-letter to 3-letters polyaminoacids code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def recode allows to translate 1-letter to 3-letters polyaminoacids code
Function `recode` translates 1-letter to 3-letters polyaminoacids code

- `main('AlaValTyr', 'DNT', method = 'recode')`
- `recode('AlaValTyr', 'DNT')`
- ![image](https://github.com/NSapozhnikov/HW4_Sapozhnikov/assets/81642791/117befa5-feaa-433a-9ac9-23cffe9b024f)
***
Copy link
Member

Choose a reason for hiding this comment

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

Тут между примерами имхо эти линии не нужны, а то совсем README получается рсчерченым в линеечку

Comment on lines +514 to +519
Args:
- *args - are supposed to be all sequences to process
- method is a kwarg - the method to process with.

Returns:
function_result - a dictionary with the result of a chosen function
Copy link
Member

Choose a reason for hiding this comment

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

👍
Очень хорошое решение убрать белки в позиционные аргументы а метод сделать именованным. Супер.

Comment on lines +523 to +524
print(f'Your sequences are: {seqs_list}',
f'The method is: {method}', sep='\n')
Copy link
Member

Choose a reason for hiding this comment

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

Ну это не уверен что нужно. А если сиквенсов много? Вы же не хотите чтобы человек прям все перечитывалт Тогда это можно и убрать.

print(f'Your sequences are: {seqs_list}',
f'The method is: {method}', sep='\n')

match method:
Copy link
Member

Choose a reason for hiding this comment

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

Тут какие то лишние пустые строки везде)

Comment on lines +531 to +532
for seq in seqs_list:
recode_dict[seq] = recode(seq=seq)
Copy link
Member

Choose a reason for hiding this comment

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

Не совсем понял. У вас часть функций типа recode принимает 1 белок и у вас цикл по белкам тут. А часть функций типа молекулярной массы и pI - принимают все белки и цикл внутри них. Имхо лучше первый вариант, но в любом случае должно быть консистентно


case 'count_gc_content':

return count_gc_content(*seqs_list)
Copy link
Member

Choose a reason for hiding this comment

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

Мне кажется лучше везде делать result = ... и потом делать return result. Просто, мало ли вы захотите как то еще добавить обработку результата, или еще что. Один return в конце мне кажется чуть более аккуратно и проще масштабируется.

Copy link
Member

@nvaulin nvaulin left a comment

Choose a reason for hiding this comment

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

Привет!

❗️ Напоминание напомнить всем членам команды посмотреть фидбек.

Классная работа! Общие моменты:

  1. Структура коммитов где то хорошая, а где то не очен.
    • Вот это хороший коммит Add local alignment function
    • Вот этот стоило бы разбить Add local alignment functionality
    • Некоторые коммиты называются с маленькой буквы
    • У этого коммита комментарий не соотвествует названию фунции (при чем я не вижу коммитов сообщение которых соотвествует переименованию). Add gc_content function
  2. Были небольшие проблемы со структурой репозитория. В данном случае .gitignore не надо было добавлять в git. Тем не менее хорошо что вы их пофиксили и очистили все ненужные файлы. Молодцы! (но в следующий раз сразу будьте аккуратнее)
  3. README хорошое. Здорово что описаны все примеры и опции использования. Ссылка на алгоритм это правильно. Тем не менее, можно было бы, например, не делать столько горизонтальных линий. Все таки у вас они в основном там, где по идее нужен просто bullet-list. Здорово что описали список авторов и зон отвественности.
  4. По коду - в основном хороший код.
    • Почти везде чистый, были пару моментов с плохими неймингами, лишними пустыми строками, пробелами и .т.д.
    • Отдельно хочу подчеркнуть понятие консистентности. Даже если можно сделать по-разному, в рамках одного проекта лучше stick to один какой-то способ.
    • Круто что сделали словари-константы. Хорошие докстринги, для внутренних функций я даже ожидал бы чуть менее подробные. В общем молодцы.
    • Есть кое-где какие-то неочевидные вещи, которые я старался прокомментировать.
    • С общим дизайном тоже все хорошо но не идеально. Так, у вас некоторые функции итерируются по белкам, а некоторые принимают только один белок. Где-то у вас повторяются проверки, хотя есть специальная функция для этого. Ну и функция эта кажется не только делает проверки, но и как-то обрабатывает ввод, что уже не очень хорошо. А еще какие то функции у вас ждут однобуквенный код, а какие-то трехбуквенный. Вот тут я совсем потрялся. Хорошо что они могут конвертировать в нужный себе формат, но опять же, почему не консистентно?

Комментарии по функциям:

  1. Функция recode возвращает ответ в виде словаря, но как-то странно.
    Там {3-letter code : 1-letter code}
    То есть запустив код main('MNT', method = 'recode') я бы ожидал получить MetAsnThr, или хотя бы {'MNT': 'MetAsnThr'} (запрос : ответ). А у вас получается наоборот. Я вашу логику понимаю, но я бы такого от тула никогда б не ожидал))

  2. main('MNT', 'MNT', method='local_alignment') дает ошибку (local variable 'seq_on' referenced before assignment). При чем кажется падает на чекере. Проверьте.
    И еще кажется я не могу подать однобуквенное выравнивание на вход алайнера. И еще когда я вызываю на двух одинаковых посдедовательностях, он мне почему-то выдает выравнивание лишь одной буквы))

main('AlaValTyrAlaValTyr', 'AlaValTyrAlaValTyr', method='local_alignment')
A
|
A
  1. Если я подаю на запрос, например, M, то я получаю ответ для Met. Тоже не хорошо. Я получается должен знать, что сделать запрос я могу в любой форме, но чтобы получить результат мне надо че то еще перекодировать ( в смысле чтобы достать ответ из словаря). В общем не очень хорошо. Я как попросил так и хочу получить ответ. Скорее всего у меня там же пайплайн какой то. Ну и + когда я отдаю один белок на вычисление массы, я бы сразу хотел получить число, а не словарь с числом. Как в домашке по ДНК РНК.

Баллы.

  • README 2.4/2.5
  • local_alignment 0.5/1.5 (круто, но не работает))
  • Остальные функции 4*1.3/1.5 = 5.2/6 (см. комменты по формату вывода)
  • За структуру коммитов -0.1
  • За проблемы с пустыми строками / обрывами строк / пробелами -0.2
  • За проблемы с неймингами кое-где -0.3
  • За небольшие проблемы с общим дизайном, логикой и консистентностью -0.3

За 2 другие поставлю по +0.4 т.к. они работают. В любом случае, в зачет идут только 5, это небольшой бонус, т.к. вы потеряли много на выравнивателе. С одной стороны, здорово что попробовали, с другой же лучше выдавать надежно готовый продукт.

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

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.

3 participants