Add abstraction for (SBML) models #133
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| MODEL_TYPE_SBML = 'sbml' | ||
|
|
||
| known_model_types = {MODEL_TYPE_SBML} |
There was a problem hiding this comment.
Should this be here or the constants file?
There was a problem hiding this comment.
I'd prefer keeping it more modular, but could consider importing it in petab.C
| 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 |
Co-authored-by: Dilan Pathirana <[email protected]>
Co-authored-by: Dilan Pathirana <[email protected]>
| @@ -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]]: | |||
There was a problem hiding this comment.
does it ever make sense to pass anything but PARAMETER_SEPARATOR here?
There was a problem hiding this comment.
Probably not for now. If the separator ever changes, it can be useful, but shouldn't hurt keeping the optional argument.
There was a problem hiding this comment.
but why is it optional for this specific function (in contrast to all the other code, where it isn't optional)?
There was a problem hiding this comment.
Because adding it to the whole potential call stack will be a mess 🙈
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.Modelusingpetab.models.Modeland replaces the respective function arguments. In the (what I consider) most relevant function for downstream use, thesbml_modelargument is kept, but deprecated.