-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add markov model #8
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,608 @@ | |||
BGC,ORF,A-ID,M domain,L-/D- (E domain),PRED_TOP5,START_POS,END_POS,STRAND,STRUCTURE ID,rBan STRUCTURE,rBan VERTEX,rBan AA-ID,rBan STRUCT_CONFIGURATION,rBan AA,STRUCTURE,VERTEX,AA-ID,MODIFICATION,AA |
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.
А этот файл стоит в репозиторий добавлять. Мне кажется вопрос стоит ли добавлять этот файл в репозиторий нужно обсуждать отдельно, вне этого пул реквеста. Или у тебя дальнейший код как-то сильно на нем завязан?
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.
У меня там дальше через этот файл оцениваются параметры, вот например в MaximumLikelihoodParametersEstimator
help="number of Baum-Welch iterations") | ||
alternative_model_group.add_argument("--log_alignments", type=bool, default=True, | ||
help="pretty log alignments with marginal probabilities or not") | ||
alternative_model_group.add_argument("--topk", type=list, default=[1, 3, 5, 10], |
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.
Мне кажется опять же, это скорее не кусок Нерпы, а отдельные скрипты должны быть, которые это всё считает. Но мне кажется это стоит отдельно обсудить, как это лучше сделать.
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.
Можно обсудить. Но мне в любом случае для выполнения своего кода нужна папочка с результатами нерпы. Чтобы из них данные NRP и BGC парсить, и чтобы с ней результаты сравнивать)
nerpa.py
Outdated
help="use Baum-Welch for parameters estimation or not") | ||
alternative_model_group.add_argument("--bw_iters", type=int, default=10, | ||
help="number of Baum-Welch iterations") | ||
alternative_model_group.add_argument("--log_alignments", type=bool, default=True, |
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.
Этот аргумент я не очень понимаю что значит.
@@ -321,6 +339,13 @@ def run(args, log): | |||
"--threads", str(args.threads)] | |||
log.info("\n======= Nerpa matching") | |||
nerpa_utils.sys_call(command, log, cwd=output_dir) | |||
if args.use_alternative_model: |
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.
Мне кажется всё-таки логичнее запускать что-то одно из этих двух в зависимости от параметров а не то и другое сразу.
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,6 @@ | |||
pandas |
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.
Полезный файлик :) Спасибо, что добавила :)
src.markov_probability_model.main.run( | ||
data_dir=output_dir, prob_gen_filepath=os.path.join(nerpa_init.configs_dir, 'prob_gen.cfg'), | ||
results_dir=os.path.join(output_dir, 'markov_probability_model_results'), | ||
mibig_path=os.path.join(nerpa_init.nerpa_root_dir, 'data', 'mibig.csv'), |
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 run(data_dir: str, prob_gen_filepath: str, | ||
results_dir: str, mibig_path: str, pool_sz: int, algo: List[str], | ||
use_bw: bool, bw_iters: int, log_alignments: bool, topk: List[int]): | ||
print('Starting alignments generation using Hidden Markov Model...') |
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.
У нас логгирование происходит немного другим способом. С помощью определнного класса, где пишется log.info. Мне кажется имеет смысл всё логгировать единообразно. Но это мелочи.
from typing import List, Dict | ||
|
||
|
||
def run(data_dir: str, prob_gen_filepath: str, |
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.
Ух ты! Типы у аргументов на питоне. Какая мило-та :))
os.makedirs(folder) | ||
|
||
print(' Loading Mibig alignments...') | ||
ground_truth_alignments: List[PairwiseAlignmentOutputWithLogs] = \ |
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.
Ух ты, а это переменная с типом. Прикольно, не видела до этого такую конструкцию в Питоне.
log_dir=res_parameters_folder).calculate_parameters() | ||
|
||
if use_bw: | ||
parameters = BaumWelchParametersEstimator(ground_truth_alignments, data, prob_gen_filepath, |
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.
Верно ли я понимаю, что если use_bw включено, тогда результат работы MaxLikelihoodParametersEstimatorWithModifications уже не нужен? Если да, то может в этом случае его и считать не стоит, зачем делать лишнюю работу?
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.
Там parameters из MaxLikelihoodParametersEstimatorWithModifications используется как начальное приближение параметров для BaumWelchParametersEstimator (в его аргументах в этой строчке), так что тут вроде все норм)
src/markov_probability_model/parameters/ml_parameters_estimator.py
Outdated
Show resolved
Hide resolved
return tau, mu | ||
|
||
|
||
def get_tau_mu_from_nerpa_cfg(nerpa_cfg: Dict) -> Tuple[np.ndarray, np.ndarray]: |
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.
Вообще так оценивать переходы звучит грубова-то конечно. Но это в целом не особо важная функция, поэтому ладно. Дальше, думаю, её можно будет в целом полностью выпилить.
# 1. Estimate g(a, mod, meth) = P(a, mod, meth | match) | ||
# 2. Estimate f(a, mod1, meth1, mod2, meth2) = P(a, mod1, meth1, mod2, meth2 | mismatch) | ||
f, g = {}, {} | ||
for modification1 in ['@L', '@D']: |
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.
У нас на самом деле тут бывают варианты L, D, не знаю.
from collections import defaultdict | ||
|
||
|
||
class MaxLikelihoodParametersEstimator: |
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.
Вообще я запуталась и думала, что этот класс должен оценивать параметры без учёта модификаций.
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.