Feature fixedpars #37

yannikschaelte · 2018-08-31T14:42:10Z

Enable fixing of parameters:

In pypesto.Problem, fixed parameters can be specified via x_fixed_indices and x_fixed_vals. This information is then propagated via the callback method objective.handle_x_fixed() to the pypesto.Objective (which is copied) which can then take reduced vectors and internally map them back to the full input vector including fixed dimensions.
In this way, the Optimizer only sees the reduced vectors, which is desirable.
After the optimization, in the results object all vectors are mapped back to include fixed dimensions, using comfort function in pypesto.Problem.
The AmiciObjective one might (later) want to change to use amici's ability to fix parameters, this has not been done yet because the AmiciObjective will undergo diverse changes anyway.
Question: E.g. problem.lb, ub are maintained in the reduced dimension. One might want to do this differently. Atm, if one want the full vectors, one would need to call problem.get_full_vector(problem.lb, problem.x_fixed_vals)

dweindl · 2018-08-31T15:26:51Z

I'd prefer if somebody who was using this feature in matlab before could review this. Who?

codecov-io · 2018-08-31T15:37:48Z

Codecov Report

Merging #37 into master will decrease coverage by 0.59%.
The diff coverage is 79.31%.

@@            Coverage Diff            @@
##           master      #37     +/-   ##
=========================================
- Coverage   82.94%   82.35%   -0.6%     
=========================================
  Files          13       15      +2     
  Lines         604      918    +314     
=========================================
+ Hits          501      756    +255     
- Misses        103      162     +59

Impacted Files	Coverage Δ
pypesto/__init__.py	`100% <100%> (ø)`	⬆️
pypesto/objective/__init__.py	`100% <100%> (ø)`
pypesto/optimize/__init__.py	`100% <100%> (ø)`	⬆️
pypesto/visualize/parameters.py	`96.77% <100%> (+0.1%)`	⬆️
pypesto/result.py	`77.41% <100%> (ø)`	⬆️
pypesto/optimize/startpoint.py	`46.87% <38.46%> (-40.63%)`	⬇️
pypesto/objective/amici_objective.py	`61.73% <61.73%> (ø)`
pypesto/problem.py	`67.9% <66.66%> (-32.1%)`	⬇️
pypesto/objective/objective.py	`85.55% <85.55%> (ø)`
pypesto/optimize/optimize.py	`90.9% <88.88%> (+21.67%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e121072...af57153. Read the comment docs.

dweindl

Looks good and convenient to use

dweindl · 2018-08-31T16:29:30Z

doc/contribute.rst

@@ -21,5 +21,20 @@ Tests can be written with `pytest <https://docs.pytest.org/en/latest/>`_
 or the `unittest <https://docs.python.org/3/library/unittest.html>`_ module.




Is contribute.rst recognized by github? Otherwise change to CONTRIBUTING.md to show this information in pull requests?

dweindl · 2018-08-31T16:41:39Z

pypesto/problem.py

+
+        if x_fixed_vals is None:
+            x_fixed_vals = np.array([])
+        self.x_fixed_vals = np.array(x_fixed_vals)


Check that len(x_fixed_vals) == len(x_fixed_indices) ?

yannikschaelte · 2018-09-05T00:55:57Z

SORRY FOR THE BIG MERGE!!!

Merging with another pull request where, among other things, the objective was enabled to record a history, a couple of more changes became necessary:

An ObjectiveHistory object was introduced which separated the history functionality from the objective class. It would be of interest to further this separation. There are different update/reset methods (one might see these as external configurators) in the objective which enable the problem and the optimization routine to make changes to these values.
objective.has_fval etc. properties were introduced which compute the logic to see what the objective function supports regarding gradients, residuals and so on. These properties are now used in the Optimizers to feed their routines with available function values. It would be desirable to be a bit more flexible here with regard to finite differences. Beforehand there must have been a logic error somewhere, see also optimization seems to fail #41.
Certain parameters were reshuffled and put into options objects, which enables easier tossing around.
The optimize.minimize function was decomposed to make it better readable.
One design point: Now everywhere x instead of par (or pars or whatever) is used. Just because it's shorter.

One remark on the underlying idea: Objective should be rather free from dimensions, because in particular different parameter fixings can be used, and the dimensions can be read automatically from the data (bounds etc) passed to a Problem. Therefore, the problem deals with everything dimension-related, and just updates the objective accordingly. It also provides functions to switch between representations, as well as the dimension used in the optimization etc.

Please indicate if you would do sth different, e.g. the options constructs or whatever.

yannikschaelte · 2018-09-05T00:57:42Z

@dweindl @FFroehlich maybe you can have a short look bc due to the merge many changes were required. I will merge to master tomorrow then hopefully. the problems with the notebooks should be resolved.

FFroehlich · 2018-09-05T01:23:15Z

pypesto/objective/objective.py

+        ]
+
+        # check whether to append to trace
+        if not self.options.trace_all and fval >= self.fval_min:


we could check for that a bit earlier, right?

At this point one remark: I found that pd.DataFrame.toc appending is really slow compared to list.append() (like 10s to <1s), but of course has better readability. Usually won't matter for our ODE problems.

FFroehlich · 2018-09-05T01:33:36Z

pypesto/optimize/optimize.py

+        print(('start ' + str(j_start) + ' failed: {0}').format(err))
+        optimizer_result = recover_result(objective, startpoint, err)
+        return optimizer_result
+    raise err


does this properly rethrow the error with full traceback? If not, I would prefer the inline version.

true, the stacktrace is wrong, starting in the new method. will change that.

dweindl

Looks good. Had only a superficial look because of time-constraints though.

dweindl · 2018-09-05T10:08:42Z

pypesto/objective/amici_objective.py

+from .objective import Objective
+
+try:
+    import amici


Here I would add a version check as mentioned in #40 . The AMICI python interface is likely to undergo some more changes, and I think it's quite annoying for the user to find out which version works

requires AMICI-dev/AMICI#426, then we should test that here using amici.__version__ and see here: https://stackoverflow.com/questions/11887762/how-do-i-compare-version-numbers-in-python

dweindl · 2018-09-05T10:13:00Z

pypesto/objective/amici_objective.py

+                    return self.get_error_output(sensi_orders, mode)
+
+                self.preequilibration_edata[fixedParameters]['x0'] = \
+                    rdata['x0']


Why are we using x0 here? Why are we not using preequilibration inside amici?

@FFroehlich ?

dweindl · 2018-09-05T10:16:01Z

pypesto/objective/objective.py

+        Flag indicating whether to save the trace.
+        Default: False.
+
+    trace_file: str, optional


Do we need trace_save? Is this not implied by trace_file?

dweindl · 2018-09-05T10:17:43Z

pypesto/objective/objective.py

+
+    def _save_trace(self, finalize=False):
+        """
+        Save to file via pickle if options.trace_save is True and other


pickle? pandas?

Fix rule scanning for observables and sigma

yannikschaelte added 21 commits August 22, 2018 15:38

git: ignore subdirs in doc/example

9d89963

init fixedpars

21e1fd6

Merge branch 'feature_plotwaterfall' into feature_fixedpars

051e2e8

make objective.call_mode_fval etc private

5f95aaf

Merge branch 'fixes_scipy' into feature_fixedpars

5e875e3

Merge branch 'master' into feature_fixedpars

d176aa7

preprocess done

e128208

rewire fixed parameters

112abb6

fix bounds [1,:]

29b4df7

fix residue dim_full map

858c566

add fixed pams notebook

5606b37

add to doc

7d01aa5

correct doc/README.md

c824621

fix typo

7065317

minor edit fixed_parameters.ipynb

210fb91

rerun rosenbrock.ipynb

b76b058

doc: drop note on using numpy

16ec1c0

transform to ndarray in OptimizerResult

d283c7b

minor edit

5316153

Merge branch 'master' into feature_fixedpars

d7929cc

viz.parameters: fix flatten bounds

962defc

yannikschaelte requested review from FFroehlich and dweindl August 31, 2018 14:42

yannikschaelte self-assigned this Aug 31, 2018

remove assert statements, change comment format for codacy

30f3817

ICB-DCM deleted a comment Aug 31, 2018

dweindl approved these changes Aug 31, 2018

View reviewed changes

yannikschaelte added 5 commits September 5, 2018 01:08

docstring for objective

e0c1da5

fix codacy lambda my not be necessary nonsense. namely it is necessary

93e9b50

fix codacy unused import

3c001b9

fix codacy get_x_names classmethod

8f38531

fix OptimizeOptions, minor edits

b1fe53d

ICB-DCM deleted a comment Sep 4, 2018

yannikschaelte added 3 commits September 5, 2018 02:11

add fixed parameters test

31005b0

Merge branch 'master' into feature_fixedpars

0c84e73

fix flake8

79c54e7