Skip to content

Commit

Permalink
Fix get_model_settings for initial states + sensitivities (#1751)
Browse files Browse the repository at this point in the history
Fixes a couple of issues in `amici.{get,set}_model_settings`

* Initial states and initial state sensitivities should only be included if custom values have been set. Otherwise, `set_model_settings(model, get_model_settings(model))` will permanently set the given values, preventing evaluation of `Model::fx0` / `Model::fsx0`.

  Fixes ICB-DCM/pyPESTO#837

* If a model had custom initial state sensitivities set, those would not have been set correctly by `set_model_settings(model, get_model_settings(model))`, because `setParameter{List,Scale}` was called after `setInitialStateSensitivities` and cleared initial state sensitivities. (Even if they would have been set, they would have been wrong in case of scaled parameters, due to using `setInitialStateSensitivities` instead of `setUnscaledInitialStateSensitivities`).
  • Loading branch information
dweindl authored Apr 4, 2022
1 parent 5634d37 commit 3d4975c
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 8 deletions.
17 changes: 14 additions & 3 deletions python/amici/swig_wrappers.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,16 +172,18 @@ def writeSolverSettingsToHDF5(
# is a tuple where the first and second elements are the getter and setter
# methods, respectively.
model_instance_settings = [
# `setParameter{List,Scale}` will clear initial state sensitivities, so
# `setParameter{List,Scale}` has to be called first.
'ParameterList',
'ParameterScale', # getter returns a SWIG object
'AddSigmaResiduals',
'AlwaysCheckFinite',
'FixedParameters',
'InitialStates',
'InitialStateSensitivities',
('getInitialStateSensitivities', 'setUnscaledInitialStateSensitivities'),
'MinimumSigmaResiduals',
('nMaxEvent', 'setNMaxEvent'),
'Parameters',
'ParameterList',
'ParameterScale', # getter returns a SWIG object
'ReinitializationStateIdxs',
'ReinitializeFixedParameterInitialStates',
'StateIsNonNegative',
Expand All @@ -203,6 +205,15 @@ def get_model_settings(
settings = {}
for setting in model_instance_settings:
getter = setting[0] if isinstance(setting, tuple) else f'get{setting}'

if getter == 'getInitialStates' and not model.hasCustomInitialStates():
settings[setting] = []
continue
if getter == 'getInitialStateSensitivities' \
and not model.hasCustomInitialStateSensitivities():
settings[setting] = []
continue

settings[setting] = getattr(model, getter)()
# TODO `amici.Model.getParameterScale` returns a SWIG object instead
# of a Python list/tuple.
Expand Down
52 changes: 47 additions & 5 deletions python/tests/test_swig_interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def test_copy_constructors(pysb_example_presimulation_module):
(10.0, 9.0, 1.0, 0.0, 0.0, 0.0),
tuple([.1]*6),
],
'InitialStateSensitivities': [
('getInitialStateSensitivities', 'setUnscaledInitialStateSensitivities'): [
tuple([1.0] + [0.0]*35),
tuple([.1]*36),
],
Expand Down Expand Up @@ -143,7 +143,6 @@ def test_model_instance_settings(pysb_example_presimulation_module):
if name != 'ReinitializeFixedParameterInitialStates'
)


# All default values are as expected.
for name, (default, custom) in model_instance_settings.items():
getter = name[i_getter] if isinstance(name, tuple) else f'get{name}'
Expand All @@ -165,8 +164,15 @@ def test_model_instance_settings(pysb_example_presimulation_module):
# The new model has the default settings.
model_default_settings = amici.get_model_settings(model)
for name in model_instance_settings:
assert model_default_settings[name] == \
model_instance_settings[name][i_default]
if (name == "InitialStates" and not model.hasCustomInitialStates())\
or (name == ('getInitialStateSensitivities',
'setUnscaledInitialStateSensitivities')
and not model.hasCustomInitialStateSensitivities()):
# Here the expected value differs from what the getter would return
assert model_default_settings[name] == []
else:
assert model_default_settings[name] == \
model_instance_settings[name][i_default], name

# The grouped setter method works.
custom_settings_not_none = {
Expand Down Expand Up @@ -299,7 +305,7 @@ def test_unhandled_settings(pysb_example_presimulation_module):
'setParameterByName',
'setParametersByIdRegex',
'setParametersByNameRegex',
'setUnscaledInitialStateSensitivities',
'setInitialStateSensitivities',
]
from amici.swig_wrappers import model_instance_settings
handled = [
Expand Down Expand Up @@ -361,3 +367,39 @@ def set_val(obj, attr, val):
)
else:
setattr(obj, attr, val)


def test_model_instance_settings_custom_x0(pysb_example_presimulation_module):
"""Check that settings are applied in the correct order, and only if
required"""
model = pysb_example_presimulation_module.getModel()

# ensure no-custom-(s)x0 is restored
assert not model.hasCustomInitialStates()
assert not model.hasCustomInitialStateSensitivities()
settings = amici.get_model_settings(model)
model.setInitialStates(model.getInitialStates())
model.setUnscaledInitialStateSensitivities(
model.getInitialStateSensitivities())
amici.set_model_settings(model, settings)
assert not model.hasCustomInitialStates()
assert not model.hasCustomInitialStateSensitivities()
# ensure everything was set correctly, and there wasn't any problem
# due to, e.g. interactions of different setters
assert settings == amici.get_model_settings(model)

# ensure custom (s)x0 is restored
model.setInitialStates(model.getInitialStates())
model.setParameterScale(amici.ParameterScaling.log10)
sx0 = model.getInitialStateSensitivities()
model.setUnscaledInitialStateSensitivities(sx0)
assert model.hasCustomInitialStates()
assert model.hasCustomInitialStateSensitivities()
settings = amici.get_model_settings(model)
model2 = pysb_example_presimulation_module.getModel()
amici.set_model_settings(model2, settings)
assert model2.hasCustomInitialStates()
assert model2.hasCustomInitialStateSensitivities()
assert model2.getInitialStateSensitivities() == sx0
assert settings == amici.get_model_settings(model2)

0 comments on commit 3d4975c

Please sign in to comment.