Hw4 voskoboinikov by wwoskie · Pull Request #27 · Python-BI-2023/HW4_Functions2

wwoskie · 2023-10-01T12:00:09Z

No description provided.

Hw4 vasilev

Hw4 lisitsa

Hw4 vasilev

…s, convert_aa_name Remove '/ Args' in all 3 functions

Update docstrings and comments

…ns2 into HW4_Vasilev

Hw4 vasilev

… into HW4_Voskoboinikov

Update README.md

albidgy

Для проверки выбрала случайных 5 функций: 'get_length_of_protein', 'get_atomic_mass', 'get_protein_rnas', 'get_length_of_protein', 'calculate_protein_mass'.
Плюсы:

Видно, что вы много времени потратили на работу.
Код написан в целом аккуратно. Но почему-то воспринимается достаточно тяжело, попробуйте упростить вашу логику, это можно сделать более легко читаемым, но нужно посидеть над кодом.
Достриги подробные, здорово.
Отличные комментарии к коммитам.
Здорово, что использовали константы.

Замечания:

В ИБ мат запрещен. Да и в принципе, если вы пишете какой-то код, который могут использовать другие люди, сохраняйте деловой стиль. Я так поняла, вы просто не дочистили код.
У вас есть возможность передавать данные в виде словаря, но, как именно это нужно сделать, не сказано.
Нет описания, что такое аргумент check_if_user_conscious. По большей части, этот аргумент только мешает нормальной работе функций. Я не нашла, где бы он использовался по назначению.
У вас многие функции достаточно не интуитивные, не всегда хватает описания функции. Например, get_frameshift_proteins.
Не нужно константы записывать в новые переменные в теле функции. Их можно вызвать напрямую.
Не используйте конкатенацию строк.
Если вы хотите выводить сообщения, что что-то пошло не так нужно их делать более информативными и не записывать в результат, а выводить на экран (то есть либо делать exceptions, либо печать на экран сообщение - лучше всего print(message, file=sys.stderr), а в результат записывать, например, None).

Баллы:

README - 2.7 балла (-0.5 за недостаточно подробное описание функций, -0.2 за отсутствие примера, как подавать словари и что в них должно быть) = 2 балла.
Функции - 7.5 баллов (-1 за конкатенацию строк, -0.7 за неправильную передачу данных в get_atomic_mass, -0.3 за запись констант в переменные внутри функций) = -2 балла.
Доп штраф за мат -1 балл.

Итого: 7.2 балла

albidgy · 2023-10-05T09:48:37Z

+    {frame_0: ['protein_seqs'], frame_1: ['protein_seqs'], frame_2: ['protein_seqs']}
+    """
+
+    if check_if_user_conscious:


Зачем здесь эта проверка? Какую смысловую нагрузку несет? Фактически программа просто останавливает работу, если не указать этот параметр. Не лучшее решение.

albidgy · 2023-10-05T09:48:40Z

+            for codon in codons:
+                for kmer in kmers:
+                    current_kmers.append(kmer + codon)  # append every codon to existing kmers


Конкатенация строк - долгий и не оптимальный процесс, лучше использовать добавление элемента к списку.

albidgy · 2023-10-05T09:48:49Z

+    if aa_atomic_mass is None:
+        aa_atomic_mass = AA_MASS_DICT


Не нужно так делать. Словарь можно вызывать напрямую.

albidgy · 2023-10-05T09:48:51Z

+    total_mass = 0
+    char = 0  # idx init
+    if atomic_mass is None:
+        atomic_mass = ATOMIC_MASS


Можно словарь вызывать напрямую. Не нужно его записывать в другую переменную.

albidgy · 2023-10-05T09:48:52Z

+            element = chem[char]
+            char += 1  # очень надо, а то я опять бесконечный цикл сделала
+            if char < len(chem) and chem[char].isdigit():
+                number = ''


Лучше не использовать конкатенацию строк, а добавлять элемента в список.

А можно вопрос, для случая с глюкозой допустим C6H12O6, как обойти конкатенацию, просто не очень понимаю. Спасибо.

Вообще сам по себе этот код неоптимальный. Но быстро я не придумала, как его аккуратнее переписать. Я подумаю на выходных, как можно иначе написать.
Касаемо конкатенации строк, лучше сделать так:

number = [] while char < len(chem) and chem[char].isdigit(): number.append(chem[char]) char += 1 # очень надо total_mass += atomic_mass[element] * int(''.join(number))

Кстати, забыла написать: комментарии лучше писать на английском языке.

albidgy · 2023-10-05T09:48:55Z

+    parsed_dct = {}
+    inp_type = type(inp)  # get input type
+    if inp_type == list:
+        for i, seq in enumerate(inp):


i - плохой нейминг, лучше ind или idx

albidgy · 2023-10-05T09:48:57Z

+
+def run_ultimate_protein_tools(command,
+                               inp,
+                               *args,


Использование *args не лучшее решение. Если пользователь подаст позиционные аргументы для другой функции не в том порядке, все сломается. Лучше просто оставить kwargs

albidgy · 2023-10-05T09:48:59Z

+            else:
+                output_dct[name] = is_protein_valid(input_dct[name])
+        elif command == 'get_atomic_mass':
+            output_dct[name] = command_dct[command](input_dct[name], *args, **kwargs)


Вы передаете в функцию get_atomic_mass **kwargs, но в самой функции **kwargs не принимаете, поэтому программа падает с ошибкой.

albidgy · 2023-10-05T09:49:35Z

+                frames_list.append(frame)  # append frame to frames list
+            frameshift_dct[f'frame_{frame_number}'] = list(set(frames_list))  # clean duplicates and write to dict
+        return frameshift_dct
+    return "You don't fucking know what you're doing!"  # politely ask user to reconsider their actions


Лучше выбрасывать ошибку и более корректную.

albidgy · 2023-10-05T09:49:39Z

+    """
+    output_dct = {}
+    input_dct = parse_input(inp, **kwargs)
+    for name in input_dct:


Здесь выдается не имя, а порядковый номер, лучше иттерироваться по значениям, чтобы выдавать последовательность. Так будет более информативно.

nvaulin · 2023-10-05T12:11:03Z

Для проверки выбрала случайных 5 функций: 'get_length_of_protein', 'get_atomic_mass', 'get_protein_rnas', 'get_length_of_protein', 'calculate_protein_mass'. Плюсы:

Видно, что вы много времени потратили на работу.

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

Достриги подробные, здорово.

Отличные комментарии к коммитам.

Здорово, что использовали константы.

Замечания:

В ИБ мат запрещен. Да и в принципе, если вы пишете какой-то код, который могут использовать другие люди, сохраняйте деловой стиль. Я так поняла, вы просто не дочистили код.

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

Нет описания, что такое аргумент check_if_user_conscious. По большей части, этот аргумент только мешает нормальной работе функций. Я не нашла, где бы он использовался по назначению.

У вас многие функции достаточно не интуитивные, не всегда хватает описания функции. Например, get_frameshift_proteins.

Не нужно константы записывать в новые переменные в теле функции. Их можно вызвать напрямую.

Не используйте конкатенацию строк.

Если вы хотите выводить сообщения, что что-то пошло не так нужно их делать более информативными и не записывать в результат, а выводить на экран (то есть либо делать exceptions, либо печать на экран сообщение - лучше всего print(message, file=sys.stderr), а в результат записывать, например, None).

Баллы:

README - 2.7 балла (-0.5 за недостаточно подробное описание функций, -0.2 за отсутствие примера, как подавать словари и что в них должно быть) = 2 балла.

Функции - 7.5 баллов (-1 за конкатенацию строк, -0.7 за неправильную передачу данных в get_atomic_mass, -0.3 за запись констант в переменные внутри функций) = -2 балла.

Доп штраф за мат -1 балл.

Итого: 7.2 балла

Амнистия за мат (всем членам команды), т.к. все таки обычная шутка
Но больше так не делайте:)

nvaulin

Всем привет еще раз, был запрос проверить другой набор функций. Запрос одобрил, функции пере-оценил (не "переоценил"), можете также найти комментарии к коду.

В следуйщий раз предусмотрим чтобы "сделать в 2 раза больше" было еще менее выиграшной стратегий. Нужно брать не количеством а качеством)

Баллы:

README 2.7
is_protein_valid 1.5/1.5
get_protein_rnas_number 1.5/1.5
count_aa 1.4/1.5
get_length_of_protein 0.75/1
get_fracture_of_aa 1.4/1.5

Итого 9.25

nvaulin · 2023-10-19T10:30:09Z

+def is_protein_valid(seq: str) -> bool:
+    """
+    Checks if protein is valid.
+
+    Arguments:
+    - seq (str): seq to be checked
+
+    Return:
+    - bool, the result of the check
+    """
+
+    if set(seq).issubset(RNA_AA_TABLE):
+        return True
+    return False


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

Suggested change

def is_protein_valid(seq: str) -> bool:

"""

Checks if protein is valid.

Arguments:

- seq (str): seq to be checked

Return:

- bool, the result of the check

"""

if set(seq).issubset(RNA_AA_TABLE):

return True

return False

def is_protein_valid(seq: str) -> bool:

"""

Checks if protein is valid.

Arguments:

- seq (str): seq to be checked

Return:

- bool, the result of the check

"""

return set(seq).issubset(RNA_AA_TABLE):

nvaulin · 2023-10-19T10:41:36Z

+    return "You don't know what you're doing!"  # politely ask user to reconsider their actions
+
+
+def get_protein_rnas_number(seq: int, **_) -> int:


Зачем тут **_ ?

nvaulin · 2023-10-19T10:43:13Z

+    check_upper = True
+    for letter in (set(codon)):
+        check_upper = letter.isupper() and check_upper
+    return check_upper


Suggested change

check_upper = True

for letter in (set(codon)):

check_upper = letter.isupper() and check_upper

return check_upper

return all([letter.isupper() for letter in set(codon)])

nvaulin · 2023-10-19T10:43:40Z

+    return len(seq)
+
+
+def count_aa(seq: str, aminoacids: str = None, **_) -> dict:


Suggested change

def count_aa(seq: str, aminoacids: str = None, **_) -> dict:

def count_aas(seq: str, aminoacids: str = None) -> dict:

nvaulin · 2023-10-19T10:44:49Z

+
+    Arguments:
+    - seq (str): sequence to count amino acids
+    - aminoacids (str): which amino acids to count in sequence


по смыслу это логичнее передавать списком

nvaulin · 2023-10-19T10:47:59Z

+    - dict: a dictionary with amino acids and its count
+    """
+
+    aa_dict_count = {}


Множественное число

Не надо писать тип данных в названии переменной. Мы же не пишем for i_int in range(0,10):

Suggested change

aa_dict_count = {}

aas_counts = {}

nvaulin · 2023-10-19T10:56:46Z

+    return "You don't fucking know what you're doing!"  # politely ask user to reconsider their actions
+
+
+def get_length_of_protein(seq: str, **_) -> int:


Suggested change

def get_length_of_protein(seq: str, **_) -> int:

def get_protein_length(seq: str) -> int:

Ну, конечно, тут ошибиться так-то негде:)
Я спокойно принимал эту функцию в других работах, потому что может не хватить фантазии / времени и так есть способ добить чем то простеньким. Ладно бы хотя бы оно правильно считало длину и в однобуквенном и в трехбуквенном коде. Но так-то это совсем примитивно)
Опять же, мы это принимали, но тут вы написали в 2 раза больше функций чем надо. Мы всё равно все в любом случае проверяем (а еще перепроверяем). В первую очередь это отнимает скорость и качество проверки у ваших коллег)). И теперь еще эта функция выделяется как одна из ваших лучших функций. Я не согласен. Почти все (если не все) ваши функции лучше чем эта, даже не смотря на какие-то синтаксические недочеты.
Я не буду сейчас ставить за неё 0, хотя, если честно, захотелось )

nvaulin · 2023-10-19T11:02:36Z

+    aa_dict_count = count_aa(seq, aminoacids=aminoacids)
+    aa_dict_percent = {}
+    len_of_protein = get_length_of_protein(seq)


Suggested change

aa_dict_count = count_aa(seq, aminoacids=aminoacids)

aa_dict_percent = {}

len_of_protein = get_length_of_protein(seq)

aas_counts = count_aa(seq, aminoacids=aminoacids)

aa_dict_percent = {}

protein_len = get_length_of_protein(seq)

aa_dict_percent название переменной не соотвествует содержанию, там могут лежать не проценты

nvaulin · 2023-10-19T11:02:52Z

+
+    if show_as_percentage:
+        mult = 100
+        round_var = 2


Suggested change

round_var = 2

precision = 2

nvaulin · 2023-10-19T11:12:21Z

+    else:
+        mult = 1
+        round_var = 4
+    aa_dict_count = count_aa(seq, aminoacids=aminoacids)


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

Aleksandr Voskoboinikov and others added 30 commits September 30, 2023 21:03

Initial commit

30f4992

Add const AMINOACID_DICT

4e2030e

Add function length_of_protein

2982a8b

Add function count_aa

c49f685

Add function get_fracture_of_aa

c7b2e9a

Merge pull request #1 from ArtemVaska/HW4_Vasilev

4fa8efd

Hw4 vasilev

Add constant H2O_WEIGHT and constant AA_MASS_DICT

5167de7

Add function calculate_protein_mass

1b3b857

Add constant ATOMIC_MASS

939b88b

Add function get_atomic_mass

db338c4

Add constant AA_NAME_DICT

8543c26

Add function convert_aa_name

f9a61da

Merge pull request #2 from ttnlsc/HW4_Lisitsa

68995b2

Hw4 lisitsa

Add a photo of the team

f97b6c3

Add README.md

bbc5a4c

Update a link of the photo in README.md

27b3885

Merge pull request #3 from ArtemVaska/HW4_Vasilev

9028075

Hw4 vasilev

Add RNA_AA_TABLE const

4e56561

Add RNA_CODON_TABLE const

55940d2

Add read_seq_from_fasta func

ca02a05

Add get_sites_lengths func

6e1d3b9

Add invert_dct func

6e03372

Add find_sites func

ff0b3ab

Add get_protein_rnas func

c64046b

Add get_protein_rnas_number func

96a0ad7

Add is_protein_valid func

6c8c71b

Add get_frameshift_proteins func

2c33705

Add COMMAND_DCT dictionary

7e64e8c

Add run_ultimate_protein_tools func

af13003

Add docstrings for read_seq_from_fasta and get_sites_lengths

02f8fde

Aleksandr Voskoboinikov and others added 27 commits October 1, 2023 12:04

Update RNA_AA_TABLE and RNA_CODON_TABLE consts with lowercase

96bccd7

Update docstrings and comments

0536237

Add check_all_upper finc and update get_frameshift_proteins

af4d819

Change name of function length_of_protein to get_length_of_protein

f03b111

Fix lowercase recognition

0f0cbcc

Update the docstring of function get_fracture_of_aa

f373a16

Update docstrings of functions calculate_protein_mass, get_atomic_mas…

274a695

…s, convert_aa_name Remove '/ Args' in all 3 functions

Merge branch 'HW4_Voskoboinikov' into HW4_Vasilev

f16bd40

Merge pull request #4 from ArtemVaska/HW4_Vasilev

a5bff00

Update docstrings and comments

Merge branch 'HW4_Voskoboinikov' of github.com:ArtemVaska/HW4_Functio…

5a11252

…ns2 into HW4_Vasilev

Merge pull request #5 from ArtemVaska/HW4_Vasilev

9d3c8fd

Hw4 vasilev

Update command dct and fix *args in count_aa and get_fracture_of_aa

fbad641

Fix merge conflicts

10dcd10

Add 2 spaces in some comments instead of 1

71090d1

Update README.md

d0e4c2e

Update run_ultimate_protein_tools and command_dct

37cc9b2

Merge pull request #6 from ArtemVaska/HW4_Vasilev

1fc7ccd

Hw4 vasilev

Update typing in consts and some func

aa60f8b

Merge branch 'HW4_Voskoboinikov' of github.com:wwoskie/HW4_Functions2…

cd79ebe

… into HW4_Voskoboinikov

Remove unused AMINOACID_DICT const

5bbd64b

Update parse_input func

b71c665

Update get_protein_rnas_number

f38ceb1

Update kwargs in input

73094a1

Update run_ultimate_protein_tools

7b0299c

Update README.md

d9aaaea

Merge pull request #8 from ArtemVaska/HW4_Voskoboinikov

14d85d5

Update README.md

Update README.md

c7db4dd

albidgy reviewed Oct 5, 2023

View reviewed changes

nvaulin reviewed Oct 19, 2023

View reviewed changes

		return "You don't know what you're doing!" # politely ask user to reconsider their actions


		def get_protein_rnas_number(seq: int, **_) -> int:

		return len(seq)


		def count_aa(seq: str, aminoacids: str = None, **_) -> dict:

	def count_aa(seq: str, aminoacids: str = None, **_) -> dict:
	def count_aas(seq: str, aminoacids: str = None) -> dict:

		return "You don't fucking know what you're doing!" # politely ask user to reconsider their actions


		def get_length_of_protein(seq: str, **_) -> int:

	def get_length_of_protein(seq: str, **_) -> int:
	def get_protein_length(seq: str) -> int:

Conversation

wwoskie commented Oct 1, 2023

Uh oh!

albidgy left a comment

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!

albidgy Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nvaulin commented Oct 5, 2023

Uh oh!

nvaulin left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

albidgy Oct 6, 2023 •

edited

Loading