Add abstraction for (SBML) models #133

dweindl · 2022-04-28T09:18:47Z

Add abstraction for models. This helps to keep libsbml code closely together and to potentially accommodate non-SBML models in the future(PEtab-dev/PEtab#538).

Wraps libsbml.Model using petab.models.Model and replaces the respective function arguments. In the (what I consider) most relevant function for downstream use, the sbml_model argument is kept, but deprecated.

codecov-commenter · 2022-04-28T09:24:54Z

Codecov Report

Merging #133 (d540c47) into develop (7a0b77e) will increase coverage by 0.11%.
The diff coverage is 74.04%.

@@             Coverage Diff             @@
##           develop     #133      +/-   ##
===========================================
+ Coverage    77.14%   77.25%   +0.11%     
===========================================
  Files           26       29       +3     
  Lines         2437     2572     +135     
  Branches       579      593      +14     
===========================================
+ Hits          1880     1987     +107     
- Misses         404      428      +24     
- Partials       153      157       +4

Impacted Files	Coverage Δ
petab/lint.py	`67.55% <51.85%> (-0.30%)`	⬇️
petab/problem.py	`67.19% <64.91%> (-0.58%)`	⬇️
petab/parameters.py	`88.19% <71.42%> (+0.30%)`	⬆️
petab/models/model.py	`72.00% <72.00%> (ø)`
petab/parameter_mapping.py	`70.15% <78.26%> (-0.18%)`	⬇️
petab/models/sbml_model.py	`91.37% <91.37%> (ø)`
petab/models/__init__.py	`100.00% <100.00%> (ø)`
petab/observables.py	`100.00% <100.00%> (ø)`

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 7a0b77e...d540c47. Read the comment docs.

…ypes

dilpath

Did not look at it in a lot of detail, but looks good 👍

petab/lint.py

dilpath · 2022-05-25T12:30:55Z

petab/models/__init__.py

+MODEL_TYPE_SBML = 'sbml'
+
+known_model_types = {MODEL_TYPE_SBML}


Should this be here or the constants file?

I'd prefer keeping it more modular, but could consider importing it in petab.C

petab/models/model.py

petab/models/sbml_model.py

petab/models/model.py

petab/parameters.py

dilpath · 2022-05-25T12:59:31Z

petab/problem.py

+        measurement_files = [
+            get_path(f) for f in problem0.get(MEASUREMENT_FILES, [])]
+        # If there are multiple tables, we will merge them
+        measurement_df = core.concat_tables(
+            measurement_files, measurements.get_measurement_df) \
+            if measurement_files else None
+
+        condition_files = [
+            get_path(f) for f in problem0.get(CONDITION_FILES, [])]
+        # If there are multiple tables, we will merge them
+        condition_df = core.concat_tables(
+            condition_files, conditions.get_condition_df) \
+            if condition_files else None
+
+        visualization_files = [
+            get_path(f) for f in problem0.get(VISUALIZATION_FILES, [])]
+        # If there are multiple tables, we will merge them
+        visualization_df = core.concat_tables(
+            visualization_files, core.get_visualization_df) \
+            if visualization_files else None
+
+        observable_files = [
+            get_path(f) for f in problem0.get(OBSERVABLE_FILES, [])]
+        # If there are multiple tables, we will merge them
+        observable_df = core.concat_tables(
+            observable_files, observables.get_observable_df) \
+            if observable_files else None


Repeated code but fine as is

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

FFroehlich

👍

FFroehlich · 2022-06-22T09:57:24Z

petab/measurements.py

@@ -154,7 +154,7 @@ def get_unique_parameters(series):

 def split_parameter_replacement_list(
        list_string: Union[str, numbers.Number],
-        delim: str = ';') -> List[Union[str, numbers.Number]]:
+        delim: str = PARAMETER_SEPARATOR) -> List[Union[str, numbers.Number]]:


does it ever make sense to pass anything but PARAMETER_SEPARATOR here?

Probably not for now. If the separator ever changes, it can be useful, but shouldn't hurt keeping the optional argument.

but why is it optional for this specific function (in contrast to all the other code, where it isn't optional)?

Because adding it to the whole potential call stack will be a mess 🙈

dweindl added 13 commits May 19, 2022 21:43

Add abstraction for models

6b26c69

Use new Model in petab.Problem

796d042

__init__.py

796219b

fix merge

d200396

TYPE_CHECKING

52f0ec0

Replace most sbml_model occurrences by new Model

0736225

doc

d798652

..

9a34c74

Deprecate petab.Problem.from_files

e0b6e74

Update petab.Problem to use Model

2924c2c

Update to test_petab

507c196

Add model type constants; add type code to class; add list of known t…

7d9dae8

…ypes

Model.to_file

3d4cda5

dweindl force-pushed the abstract_model branch from 003ab5d to 3d4cda5 Compare May 19, 2022 19:43

dweindl added 5 commits May 19, 2022 21:56

fixup, doc

d8d1774

cleanup assert_model_parameters_in_condition_or_parameter_table

ee977a7

Use petab.Model in parameter mapping

c04a75d

sbml_model->model

69f826e

cleanup

9bc1e6f

dweindl requested review from fbergmann and FFroehlich May 20, 2022 12:26

dweindl marked this pull request as ready for review May 20, 2022 14:29

dweindl and others added 4 commits May 24, 2022 16:58

Merge branch 'develop' into abstract_model

4996699

fixup merge

d540c47

Merge branch 'develop' into abstract_model

78251a0

fix merge

bb61a12

dweindl requested a review from dilpath May 25, 2022 08:48

Merge branch 'develop' into abstract_model

66e1bc4

dilpath approved these changes May 25, 2022

View reviewed changes

dweindl and others added 9 commits June 20, 2022 16:15

is/are

ed7c23c

Update petab/models/model.py

4dbcfe0

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

Update petab/models/model.py

804188b

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>

the

3b337b0

import

d66e18a

fixup rename

5464928

PARAMETER_SEPARATOR

4dcf1f6

Merge branch 'develop' into abstract_model

a9bf397

..

49e518c

FFroehlich approved these changes Jun 22, 2022

View reviewed changes

FFroehlich reviewed Jun 22, 2022

View reviewed changes

dweindl merged commit e3a8f82 into develop Jun 22, 2022

dweindl deleted the abstract_model branch June 22, 2022 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add abstraction for (SBML) models #133

Add abstraction for (SBML) models #133

dweindl commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading

dilpath left a comment

dilpath May 25, 2022

dweindl Jun 20, 2022

dilpath May 25, 2022

FFroehlich left a comment

FFroehlich Jun 22, 2022

dweindl Jun 22, 2022

FFroehlich Jun 22, 2022

dweindl Jun 22, 2022

FFroehlich Jun 22, 2022

		MODEL_TYPE_SBML = 'sbml'

		known_model_types = {MODEL_TYPE_SBML}

Add abstraction for (SBML) models #133

Add abstraction for (SBML) models #133

Conversation

dweindl commented Apr 28, 2022 • edited Loading

codecov-commenter commented Apr 28, 2022 • edited Loading

Codecov Report

dilpath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FFroehlich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweindl commented Apr 28, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading