Fixes and qol features #26
Conversation
pypesto/objective.py
Outdated
| # fails this should not be caught right? | ||
|
|
||
| try: | ||
| import pandas as pd |
There was a problem hiding this comment.
I would rather put pandas in setup.py -> install_requires (might have missed that), and avoid try here, since it will be needed in many places anyway
…e use of update trace
…pdate_eval_counts
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
==========================================
+ Coverage 80.52% 82.94% +2.42%
==========================================
Files 13 13
Lines 457 604 +147
==========================================
+ Hits 368 501 +133
- Misses 89 103 +14
Continue to review full report at Codecov.
|
pypesto/objective.py
Outdated
| return sres | ||
|
|
||
| """ | ||
| Gradient checker function |
There was a problem hiding this comment.
could you describe in more detail what the function does?
pypesto/objective.py
Outdated
| @@ -81,6 +80,18 @@ def __init__(self, fun, | |||
| self.res = res | |||
| self.sres = sres | |||
|
|
|||
| self.dim = dim | |||
There was a problem hiding this comment.
we had discussed with Jan to have dim only in the problem description, because sometimes an objective can be called with different argument lengths, and because we have this whole fixed parameters stuff. i am currently also working on sth related, and introduced in Problem a full_dim and a dim field which can be filled in diverse ways.
would there be a reason to have dim in objective with amici?
| self.temp_file = None | ||
| self.temp_save_iter = None | ||
|
|
||
| def update_eval_counts(self, sensi_orders): |
There was a problem hiding this comment.
nice solution. i was wondering whether it would be possible to move this "counting stuff" to a wrapper class, so as to disentangle the objective and gradient computation from the counting part. the wrapping could then occur before every local optimization, also helping avoid parallelization and history issues.
but that can still be done later.
There was a problem hiding this comment.
Sure, we could push all the newly introduced fields to a different class but I do not see a way around attaching that class to the objective. But maybe there is some python magic that I do not know of.
Regarding parallelization, I think really the best way to do this is have pypesto being started from separate jobs. This is the only way to effectively parallelize across nodes on a cluster and also works on a local machine.
It would not work with cooperative scatter search strategies, but there we probably want to introduce an additional level of hierarchy anyways.
There was a problem hiding this comment.
nice solution. i was wondering whether it would be possible to move this "counting stuff" to a wrapper class, so as to disentangle the objective and gradient computation from the counting part.
Using decorators?
Regarding parallelization, I think really the best way to do this is have pypesto being started from separate jobs
what do you mean by separate jobs?
There was a problem hiding this comment.
Jan also mentioned that we would like to have transparent parallelization engines in optimization and sampling à-la pyabc using e.g. redis-servers, much of which one might be able to re-use. in optimization first of all I guess only starts would be parallelized, but also startpoint selection.
There was a problem hiding this comment.
what do you mean by separate jobs?
submitting individual batch jobs for every individual (subbatch of) optimization(s). e.g. using snakemake:
rule parameter_estimation:
input:
model=rules.build_amici.output,
data=rules.process_data.output,
script='parameter_estimation.py',
output:
os.path.join('results', 'multistart', '{modelname}', 'result_{dataset}_{repeat}.pickle')
wildcard_constraints:
modelname='[\w\d_]+',
dataset='[\w\d]+',
repeat='\d+'
shell:
expand('python3 {{input.script}} {{input.model}} {{wildcards.dataset}} {{wildcards.repeat}} {n_starts}', n_starts=MS_PER_JOB)[0]
This should fit with the notion of using redis servers. I would not recommend implementing threading or anything of that sorts in pypesto.
There was a problem hiding this comment.
Wait, in pyABC you probably parallelized on the level to objective function calls and not on the level of optimization runs. This of course helps with load balancing but may result in overhead when submitting millions of tiny jobs for simpler models.
… into fixes_qol_features
| self.temp_file = None | ||
| self.temp_save_iter = None | ||
|
|
||
| def update_eval_counts(self, sensi_orders): |
There was a problem hiding this comment.
nice solution. i was wondering whether it would be possible to move this "counting stuff" to a wrapper class, so as to disentangle the objective and gradient computation from the counting part.
Using decorators?
Regarding parallelization, I think really the best way to do this is have pypesto being started from separate jobs
what do you mean by separate jobs?
pypesto/objective.py
Outdated
| @@ -244,12 +453,35 @@ class AmiciObjective(Objective): | |||
|
|
|||
| def __init__(self, amici_model, amici_solver, edata, max_sensi_order=None, | |||
| preprocess_edata=True): | |||
| if 'amici' not in sys.modules: | |||
| if amici is None: | |||
| print('This objective requires an installation of amici (' | |||
There was a problem hiding this comment.
raise instead of print? No need to continue until we fail accessing amici. Maybe also better to keep everything amici-dependent in a separate file.
pypesto/optimize/optimize.py
Outdated
| err | ||
| ) | ||
| else: | ||
| optimizer_result = optimizer.minimize(problem, startpoint, j) |
There was a problem hiding this comment.
Avoid duplicate call to optimizer.minimize:
try: ...
except:
if allow_failed_starts: ..
else: rethrow
?
| suite.addTest(test_sbml_conversion.OptimizerTest()) | ||
| suite.addTest(test_sbml_conversion.AmiciObjectiveTest()) | ||
| suite.addTest(test_objective.ObjectiveTest()) | ||
| suite.addTest(test_optimize.OptimizerTest()) | ||
| testRunner = unittest.TextTestRunner(verbosity=0) | ||
| result = testRunner.run(suite) |
There was a problem hiding this comment.
is code_coverage still used? on travis coverage should be done automatically in https://github.com/ICB-DCM/pyPESTO/blob/master/.travis.yml#L39, afaio.
There was a problem hiding this comment.
You are right, could you please delete the file in your PR?
No description provided.