Hw4_Grigoriants by VovaGrig · Pull Request #19 · Python-BI-2023/HW4_Functions2

VovaGrig · 2023-10-01T00:32:03Z

No description provided.

…ions inside

Add 'search_for_alt_frames' and 'convert_to_nucl_acids' functions

…into HW4_Grigoriants

…ixes

Add three_one_letter_code and define_molecular_weight functions

…into HW4_Grigoriants

pavlovanadia · 2023-10-04T11:31:39Z

+## Usage
+The programm is based on `run_protein_tools` function that takes the list of **one-letter amino acid sequences**,  a name of procedure and a relevant argument. If you have three-letter amino acids sequences you could convert them by using `three_one_letter_code` procedure in advance. Please convert your three-letter coded sequences with `three_one_letter_code` procedure before using any other procedures on them.
+
+To start with the program run the following command:
+
+`run_protein_tools(sequences, procedure="procedure", ...)`
+
+Where:
+- sequences - positional argument, a list of protein sequences
+- procedure - keyword argument, a type of procedure to use that is inputed in *string* type
+- ... - an additional keyword arguments that are to be inputed in *string* type
+- 
+Before start, check the *Options* and *Examples*.


Супер!

pavlovanadia · 2023-10-04T11:31:43Z

@@ -0,0 +1,106 @@
+amino_acids = {


Поскольку это константа, то имя переменной должно быть в uppercase

pavlovanadia · 2023-10-04T11:31:44Z

+    "w": "trp",
+    "y": "tyr",
+}
+translation_rule = {


Поскольку это константа, то имя переменной должно быть в uppercase

pavlovanadia · 2023-10-04T11:31:46Z

+    "G": "GGC",
+    "g": "ggc",
+}
+amino_acid_weights = {


Поскольку это константа, то имя переменной должно быть в uppercase

pavlovanadia · 2023-10-04T11:31:48Z

+        if "-" not in sequence:
+            for letter in sequence:
+                inversed_sequence += dictionaries.amino_acids[letter] + "-"
+            inversed_sequence = inversed_sequence[:-1]
+            inversed_sequences.append(inversed_sequence)


Проще было бы собирать аминокислоты в список, а потом через join добавить "-"
Так и костыля inversed_sequence = inversed_sequence[:-1] не было бы

pavlovanadia · 2023-10-04T11:31:50Z

+            aa_splitted = sequence.split("-")
+            for aa in aa_splitted:
+                inversed_sequence += list(dictionaries.amino_acids.keys())[
+                    list(dictionaries.amino_acids.values()).index(aa)
+                ]
+            inversed_sequences.append(inversed_sequence)


Очень много уровней вложенности

pavlovanadia · 2023-10-04T11:31:52Z

+    for sequence in sequences:
+        sequence_weight = 0
+        for letter in sequence:
+            sequence_weight += dictionaries.amino_acid_weights[letter.upper()]


Это хорошо, если учесть, что идет обращение к единственному словарю, где все а\к записаны один раз в upper-case!

А вот вопрос. В задании нам требовалось оставлять регистр символов в исходном формате. Т. е. 'Rm' -> 'Arg-met'
А как нам сделать один словарь только в upper-case, если нам нужен чувствительный регистр?
Грубо говоря, в молекулярном весе нам в этом нет необходимости, но в остальных функциях это требуется.
И при переводе в lower- upper-case мы теряем уникальность исходного регистра.
Или есть какой-то дополнительный метод, который позволит нам это сделать?

Например, это можно сделать проверкой .isupper()

Спасибо!

pavlovanadia · 2023-10-04T11:31:53Z

+        print(f"Sequence: {sequence}")
+        print(f"Motif: {motif}")


Юзер-френдли вывод!

pavlovanadia · 2023-10-04T11:31:55Z

+                if overlapping:
+                    start += 1
+                else:
+                    start += len(motif)


Красиво!

pavlovanadia · 2023-10-04T11:31:57Z

+    rule_of_translation = sequences[0].maketrans(dictionaries.translation_rule)
+    rule_of_transcription = sequences[0].maketrans("AaUuCcGg", "TtAaGgCc")


Зачем каждый раз при вызове функций заново создавать одинаковые словари? Если делать так, то уж выносить в константу

pavlovanadia · 2023-10-04T11:32:02Z

+    for sequence in sequences:
+        rna_seq = sequence.translate(rule_of_translation)
+        dna_seq = rna_seq.translate(rule_of_transcription)
+        if nucl_acids == "RNA":


Переусложнение кода,
seq.translate(dictionaries.translation_rule)
будет работать тоже исправно, и без создания лишних словарей

У меня так не работает(
Только через maketrans переводит

Тут согласна, прошу прощения

Спасибо за замечания! Я в процессе разгребания всех этих словарей сама хоть маленько разобралась)

pavlovanadia · 2023-10-04T11:32:04Z

+            if sequence == sequences[-1]:
+                del nucl_acid_seqs["RNA"]


Ненужная проверка, можно было сразу удалить и без условия

Удалять без условия внутри цикла нельзя, потому что он удалит на первой же итерации, а уже на второй упадает с ошибкой, что удалять нечего, т.к удалил на первой итерации. Потому проверяем условие, что итерация последняя

Таких пометки было две(для РНК и ДНК), случайно зарезолвил одну😅

pavlovanadia · 2023-10-04T11:32:04Z

+        if nucl_acids == "both":
+            nucl_acid_seqs["RNA"].append(rna_seq)
+            nucl_acid_seqs["DNA"].append(dna_seq)
+    return nucl_acid_seqs


Код функции переусложнен, но идея понятная и хорошая,
Вывод очень user-friendly - круто!

pavlovanadia · 2023-10-04T11:32:07Z

+procedures_to_functions = {
+    "search_for_motifs": search_for_motifs,
+    "search_for_alt_frames": search_for_alt_frames,
+    "convert_to_nucl_acids": convert_to_nucl_acids,
+    "three_one_letter_code": three_one_letter_code,
+    "define_molecular_weight": define_molecular_weight,
+}


Надо сделать константой

pavlovanadia · 2023-10-04T11:32:08Z

+    """
+    if len(sequences) == 0:
+        raise ValueError("No sequences provided")
+    procedure = kwargs["procedure"]


Отлично!

pavlovanadia · 2023-10-04T11:32:10Z

+    allowed_inputs = set(dictionaries.amino_acids.keys()).union(
+        set(dictionaries.amino_acids.values())
+    )


Тоже хорошо!

pavlovanadia · 2023-10-04T11:32:11Z

+    if procedure != "three_one_letter_code":
+        allowed_inputs -= set(dictionaries.amino_acids.values())


Лучше бы наоборот, если procedure == "three_one_letter_code", то только тогда объединять множества, иначе получается две лишние операции для всех функций, кроме одной ( И туда же добавлять "-"

pavlovanadia · 2023-10-04T11:32:13Z

+        if procedure == "three_one_letter_code" and "-" in sequence:
+            allowed_inputs_seq -= set(dictionaries.amino_acids.keys())


Это тоже лишние операции
Лучше добавлять при необходимости, чем сначала делать большое множество, и потом из него что-то удалять

pavlovanadia · 2023-10-04T11:32:14Z

+            if not all(
+                aminoacids in allowed_inputs_seq for aminoacids in sequence.split("-")
+            ):


Т.к. allowed_inputs_seq является множеством, то не надо итерироваться по всей послед-ти и элементам множества, а достаточно использовать методы множеств:

Suggested change

if not all(

aminoacids in allowed_inputs_seq for aminoacids in sequence.split("-")

):

if not set(sequence.split("-")).issubset(allowed_inputs_seq):

pavlovanadia · 2023-10-04T11:32:15Z

+            allowed_inputs_seq.remove("-")
+            allowed_inputs_seq -= set(dictionaries.amino_acids.values())
+            if not all(aminoacids in allowed_inputs_seq for aminoacids in sequence):
+                raise ValueError("Invalid sequence given")


Концептуально те же комментарии, что были выше: ненужные операции над сетами (добавлять\удалять что-то из сетов занимает больше времени, чем в случае списков, так что по возможности надо таких операций как можно меньше делать), ненужная итерация по всем эл-там последовательности и множества, неоптимальный код

pavlovanadia · 2023-10-04T11:32:25Z

Хорошая работа!

Отличный README, правда, докстринг в него вставлять не стоит, лучше информацию о функциях писать в более user-friendly формате
Отличные примеры запуска программы, хорошо описаны ошибки. В общем, README огонь (+0.2 балла за фото команды!)
Улучшить README можно, помимо удаления докстринг, еще добавив просто общий список команд куда-то, до подробного описания каждой
Программа не работает с любым кол-вом аргументов. Если хочется ввести несколько последовательностей, то принимаются они только в списке. Одна последовательность - тоже должна быть в списке длиной 1, в общем, довольно неудобный формат ввода аргументов. Если дается одна послед-ть в виде строки, то все опции будут применяться к каждой а\к независимо, но не к последовательности. Очень хорошо, что такой формат ввода прописан в README, но это все еще не очень user-friendly
Словари dictionaries занимают в два раза больше памяти, чем могли бы, потому что все а\к там записаны и в uppercase, и в lowercase. Проще было бы в коде применять эту операцию.
Здорово, что константы вынесены в отдельный файл, но константы принять называть заглавными буквами
В коде много неоптимальных частей: лишние проверки, лишние изменения множества, итерация по элементам множества

pavlovanadia · 2023-10-06T13:34:53Z

Пересчет баллов:

README 2.4 (-докстринги, вставленные прямо в текст ридми)
three_one_letter_code - 1.2 (проблема типизации, неоптимальное добавление)
define_molecular_weight - 1.4 (типизация)
search_for_motifs - 1.5
search_for_alt_frames - 1.4 (типизация)
convert_to_nucl_acids - 1.3 (ненужные проверки в цикле)

-0.1 нейминг констант
-0.2 вид инпута
-0.2 вычитания множеств и прочее
-0.2 итерация по послед-ти и множеству
+0.2 доп балл за фото

VovaGrig · 2023-10-06T20:51:51Z

Спасибо за тщательную проверку и полезные комментарии👍

Вид инпута - супер субъективно, никаких ограничений на это не давалось от Никиты, а список это стандартизованно.

Про возможность вставки докстринга в ридми говорил Никита на лекции - потому так и сделали.

Все, что «неоптимально» - использовали то, чему Никита успел научить и что сами успели вычитать 💁🏻‍♂️

В целом, как кажется мне и знакомому преподавателю плюсов на физтехе, - снимать за такие недочеты баллы - не очень справедливо.
Можно же просто вставить как комментарии нам на будущее.

Во все формальные критерии мы уложились, все что больше - от лукавого :)

На будущее - либо продумывайте лучше формальные критерии ( например, время обработки одной последовательности для каждой из функций не должно превышать х), либо оставляйте пожелания в виде комментариев, но не снимайте баллы за то, что мы не соответствуем негласным критериям. Они неочевидны, совсем.

…into HW4_Grigoriants

VovaGrig and others added 30 commits September 27, 2023 13:10

Add folder HW4_Grigoriants, create README.md

f4861e7

Add protein_tools.py with run_protein_tools and check_for_motif funct…

c3b919c

…ions inside

Add 'search_for_alt_frames' function

bc24a41

Add 'convert_to_nucl_acids' function

f81d442

Add conditions in 'main' function

cbeb58a

Merge pull request #1 from EkaterinShitik/HW4_Grigoriants

1cd3287

Add 'search_for_alt_frames' and 'convert_to_nucl_acids' functions

Add minor fix to protein_tools.py

d91cfd4

Merge branch 'HW4_Grigoriants' of github.com:VovaGrig/HW4_Functions2 …

7f54bec

…into HW4_Grigoriants

Add check_and_parse_user_input in protein_tools.py, add fixes

39b8acd

Add minor fixes in protein_tools.py

29fd752

Add check_and_parse_user_input in protein_tools.py, add fixes

de4e146

Add three_one_letter_code and define_molecular_weight functions and f…

620a551

…ixes

Merge pull request #2 from vladislavi27/HW4_Grigoriants

c641b5e

Add three_one_letter_code and define_molecular_weight functions

Add minor fixes in protein_tools.py

93d2d5f

Merge branch 'HW4_Grigoriants' of github.com:VovaGrig/HW4_Functions2 …

ac9a165

…into HW4_Grigoriants

Add minor fixes in protein_tools.py

d731697

Add minor changes to 'convert_to_nucl_acids' function

e670429

Change transcription rule in 'convert_to_nucl_acids' function

fe41d85

Correct inaccuracies in the dockstring of 'convert_to_nucl_acids'

c8e9823

Change inaccuracies in the dockstring of 'convert_to_nucl_acids'

cb03cf4

Change annotation of 'search_for_alt_frames' function

b193a6b

Add minor fixes in protein_tools.py

f53914a

Add plan of README.md

2ce8ada

Complete 'Usage'

18c1a76

Add preliminary 'Options'

ea3be7e

Add preliminary 'Examples'

a1c1c23

Merge branch 'VovaGrig:HW4_Grigoriants' into HW4_Grigoriants

6a4e2b1

Complete 'Examples'

454d703

Complete four first parts

e5628a5

Complete all parts except for contacts

53a7556

VovaGrig added 5 commits October 1, 2023 02:12

Add mifixes to docstrings

6794624

Add mminor fixes

d3b21d1

Update README.md: add information, pictures, team photo

7412e71

Update README.md

4d23561

Update README.md

dd6f4a6

pavlovanadia reviewed Oct 4, 2023

View reviewed changes

Comment thread HW4_Grigoriants/protein_tools.py Outdated

pavlovanadia reviewed Oct 4, 2023

View reviewed changes

VovaGrig added 2 commits October 14, 2023 22:09

Add fixes based on feedback to dictionaries.py and protein_tools.py

a3bec1b

Merge branch 'HW4_Grigoriants' of github.com:VovaGrig/HW4_Functions2 …

0416e82

…into HW4_Grigoriants

		rule_of_translation = sequences[0].maketrans(dictionaries.translation_rule)
		rule_of_transcription = sequences[0].maketrans("AaUuCcGg", "TtAaGgCc")

		if procedure != "three_one_letter_code":
		allowed_inputs -= set(dictionaries.amino_acids.values())

		if procedure == "three_one_letter_code" and "-" in sequence:
		allowed_inputs_seq -= set(dictionaries.amino_acids.keys())

Conversation

VovaGrig commented Oct 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavlovanadia commented Oct 4, 2023

Uh oh!

pavlovanadia commented Oct 6, 2023

Uh oh!

VovaGrig commented Oct 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants