-
Notifications
You must be signed in to change notification settings - Fork 47
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
Visualize: visualization of estimated observable mapping #1409
Conversation
# Mask the data using the inner problem mask. This is necessary | ||
# because the inner problem is aware of only the data it uses. | ||
for i in range(len(data)): | ||
data[i][~self.data_mask[i]] = np.nan | ||
|
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.
This is a bug fix. It caused an issue only if the model contained multiple non-quantitative data types (e.g. semiquantitative and relative). In that case, this check always failed, as we were not masking the data in the check in the same way it was masked in the inner problem initialization.
|
||
self.semiquant_observable_ids = None | ||
self.relative_observable_ids = None | ||
|
||
self.construct_inner_calculators( | ||
petab_problem, model, edatas, inner_options | ||
) | ||
|
||
self.quantitative_data_mask = self._get_quantitative_data_mask(edatas) | ||
|
||
self._known_least_squares_safe = False | ||
self.semiquant_observable_ids = None |
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.
Bugfix.
semiquant_observable_ids
would be set to the right value in construct_inner_calculators
and then was always overwritten to None in line 112.
@@ -66,43 +65,35 @@ def from_petab_amici( | |||
petab_problem, amici_model, edatas | |||
) | |||
|
|||
def check_edatas(self, edatas: list[amici.ExpData]) -> bool: |
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.
Check edatas was re-defined completely the same for no reason. It is already defined in AmiciInnerProblem.
def get_relative_observable_ids(self) -> list[str]: | ||
"""Get unique list of IDs of all relative observables with scaling and/or offset.""" | ||
return list( | ||
{ | ||
x.observable_id | ||
for x in self.xs.values() | ||
if x.inner_parameter_type | ||
in [ | ||
InnerParameterType.SCALING, | ||
InnerParameterType.OFFSET, | ||
] | ||
} | ||
) | ||
|
||
for data0, data1 in zip(self.data, data): | ||
if not np.array_equal(data0, data1, equal_nan=True): | ||
return False | ||
def get_groups_for_xs(self, inner_parameter_type: str) -> list[int]: | ||
"""Get unique list of ``RelativeParameter.group`` values.""" | ||
groups = [x.group for x in self.get_xs_for_type(inner_parameter_type)] | ||
return list(set(groups)) | ||
|
||
return True | ||
def get_xs_for_group(self, group: int) -> list[RelativeInnerParameter]: | ||
r"""Get ``RelativeParameter``\s that belong to the given group.""" | ||
return [x for x in self.xs.values() if x.group == group] |
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.
Ease of life class methods.
def get_semiquant_observable_ids(self) -> list[str]: | ||
"""Get the ids of semiquantitative observables.""" | ||
return list( | ||
{ | ||
x.observable_id | ||
for x in self.xs.values() | ||
if x.inner_parameter_type == InnerParameterType.SPLINE | ||
} | ||
) | ||
|
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.
Some more ease of life
self.relative_observable_ids = ( | ||
self.objective.calculator.relative_observable_ids | ||
) |
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.
Saving which observables are relative in the pypesto problem, easier access.
# Plot the estimated observable mapping for relative observables. | ||
if n_relative_observables > 0: | ||
axes = plot_linear_observable_mappings_from_pypesto_result( | ||
pypesto_result=pypesto_result, | ||
pypesto_problem=pypesto_problem, | ||
start_index=start_index, | ||
axes=axes, | ||
rel_and_semiquant_obs_indices=rel_and_semiquant_obs_indices, | ||
) | ||
|
||
# Plot the estimated spline approximations for semi-quantitative observables. | ||
if n_semiquant_observables > 0: | ||
axes = plot_splines_from_pypesto_result( | ||
pypesto_result=pypesto_result, | ||
start_index=start_index, | ||
axes=axes, | ||
rel_and_semiquant_obs_indices=rel_and_semiquant_obs_indices, | ||
) |
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.
The main routine visualize_estimated_observable_mapping
calls the linear mapping and spline one if there are relative or semiquantitative observables, respectively. However, the two lower-level routines can also be called directly.
The same axes object is being passed around and plotted on.
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.
Looks great 👍
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.
Thanks, looks good to me.
The only concern is the one-parameter-to-one-observable mapping.
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1409 +/- ##
===========================================
- Coverage 83.85% 83.32% -0.54%
===========================================
Files 160 161 +1
Lines 13107 13226 +119
===========================================
+ Hits 10991 11020 +29
- Misses 2116 2206 +90 ☔ View full report in Codecov by Sentry. |
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.
Thanks for the implementation, only two suggestions but no need to address them.
pypesto/visualize/__init__.py
Outdated
@@ -19,6 +19,13 @@ | |||
) | |||
from .ensemble import ensemble_identifiability | |||
from .misc import process_offset_y, process_result_list, process_y_limits | |||
from .observable_mapping import ( | |||
_add_spline_mapped_simulations_to_model_fit, |
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.
Is it necessary to add that function here? It looks like it should be private
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.
True, you're right. Don't know why it was there.
Removing it.
pypesto_result: | ||
The pyPESTO result object from optimization. | ||
pypesto_problem: | ||
The pyPESTO problem. |
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.
The pypesto problem objective is used downstream and I think that we can have problems without objective so it could be nice to specify here that the pypesto_problem
is expected to have an objective
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.
True.
Added it into the docstring and added some check to check that there is an objective and that it has the correct calculator.
I implemented a visualization routine to visualize all estimated observable mappings: linear scaling+offset for relative data and splines for semiquantitative data. The visualization of estimated splines already existed, so I made the linear one and combined them into a single routine. However, I made it so that each can also be called separately.
To make the implementation easier, I had to expand some classes connected to the relative data: inner problem, inner parameter.
During the implementation, a few bugs popped up. I fixed those here as well. I'll comment on the PR so it's easier to distinguish.
The notebooks have also been updated accordingly.