Skip to content
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

HW4 Shtompel #23

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

Conversation

anshtompel
Copy link

No description provided.

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.

Привет!

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

Очень хорошая работа)

  • Понравились ваши идеи по работе с белками. Что-то совсем простое конечно, но есть прям креативные
  • Хороший README, но не хватило каких-нибудь примеров запуска которые можно прям скопировать и вставить. Когда используешь какую-то левую библиотеку всегда удобно на таких примерах проверить что у тебя все работает как надо. Инфа такая у вас конечно есть, но люди ленивые. Вот прям чтобы отдельно кодом вызов который можно скопировать и вставить.
  • Сообщения коммитов ок, но струткура не везде отличная. Где-то есть что прям куча всего одним коммитом добавлено. Это не оч хорошо.
  • По формату вывода написал еще ниже, что возможно стоило бы сделать словарем. Как минимум, если 1 белок на входе, то лучше вернуть 1 число/... (а не список из 1 числа)
  • Были какие то минорные замечания по оформлению. В целом все хорошо.
  • По структуре были функции с несколько похожим функционалом. В таком случае удобно делать одну внутренню функцию которая достает статистику в каком то очень подробном но неудобном виде. А потом эти функции лишь забирают из этой статистики то что им нужно

Баллы:

  • README 2.4/2.5
  • Операции работают, 5*1.5 = 7.5/7.5 (оценивал 5 лучших, т к в задании требовалось только 5)
  • За формат ввода и вывода -0.1
  • За некоторые избыточности в логике кода: -0.3
    Итого: 9.5

Спасибо!

Copy link
Member

Choose a reason for hiding this comment

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

Зачем-ж вы с ним так:)

@@ -0,0 +1,50 @@
# Protein_tools
Copy link
Member

Choose a reason for hiding this comment

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

Приятный хороший README! Вы все что нужно в целом расписали. Единственное, хорошо когда есть прям пример использования. Строчка кода которую можно прям сразу скопировать, вставить, и посмотреть какой будет вывод (и сравнить с тем что написано в README). Этого немного не хватает:)

### Limitations and troubleshooting
**Protein_tools** has several limitations that can raise the errors in the work of the program. Here are some of them:
1. **Protein_Tools** works only with protein sequences that contains letters of Latin alphabet (the case is not important); also every aminoacid should be coded by one letter. If there are other symbols in the sequence, the tool raise `ValueError` *"One of these sequences is not protein sequence or does not match the rools of input. Please select another sequence."*. In this case you should check if there are punctuation marks, spaces or some other symbols in your sequence.
2. Be careful to work only with the sequences that contain aminoacids that coded with one letter. If your sequense is "SerMetAlaGly", **Protein_tools** reads it as "SERMETALAGLY".
Copy link
Member

Choose a reason for hiding this comment

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

Очень хорошее замечение! Было бы здорово в версии 2.0 ввести поодержку трехбуквенной кодировки (полушутка).
Но, ведь правда, мне кажется сходну почти все помнят 3буквенную и не всё помнят из 1буквенной

1. **Protein_Tools** works only with protein sequences that contains letters of Latin alphabet (the case is not important); also every aminoacid should be coded by one letter. If there are other symbols in the sequence, the tool raise `ValueError` *"One of these sequences is not protein sequence or does not match the rools of input. Please select another sequence."*. In this case you should check if there are punctuation marks, spaces or some other symbols in your sequence.
2. Be careful to work only with the sequences that contain aminoacids that coded with one letter. If your sequense is "SerMetAlaGly", **Protein_tools** reads it as "SERMETALAGLY".
3. The list of available functions is available in section "Options". If you see `ValueError` *"This procedure is not available. Please choose another procedure."*, probably your spelling of the name of function is incorrect. Please check the name of chosen prosedure and make sure that it is available in the **Protein_Tools**.

Copy link
Member

Choose a reason for hiding this comment

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

🔥

Copy link
Member

Choose a reason for hiding this comment

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

Ну это тут наверное не нужно

'count_charge': count_charge}


def protein_tools(*args: str) -> list:
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 protein_tools(*args: str) -> list:
def run_protein_tools(*args: str) -> list:



def protein_tools(*args: str) -> list:
"""
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 +190 to +192
operation = args[-1]
parsed_seq_list = []
for seq in args[0:-1]:
Copy link
Member

Choose a reason for hiding this comment

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

Лучше по другому сделайте аргументы. То как было в ДЗ 3 н самом деле не очень удобно. Пусть белки принимаются либо позиционно либо даже одним списком, а вот операция будет именованным. Чтобы это все было не в одной куче, а отдельно.

Comment on lines +193 to +194
if not is_protein(seq):
raise ValueError("One of these sequences is not protein sequence or does not match the rools of input. Please select another sequence.")
Copy link
Member

Choose a reason for hiding this comment

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

Вот это красиво выглядит:)

parsed_seq_list.append(OPERATIONS[operation](seq))
else:
raise ValueError("This procedure is not available. Please choose another procedure.")
return parsed_seq_list
Copy link
Member

Choose a reason for hiding this comment

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

Можно кстати тут возвращать не список, а словарь. Ключ - это белок из ввода, а значение - это результат работы программы. Но это чисто как идея, и так тоже хорошо

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