Add PySBModel for handling of PySB models #145

dweindl · 2022-05-04T12:49:42Z

Draft for handling PySB models in PEtab.

adds newly introduced model IDs in yaml file to petab.Model, and handles change form sbml_files to model_files
adds support for the newly introduced model<->petab entity mapping file

codecov-commenter · 2022-05-24T15:14:36Z

Codecov Report

Merging #145 (91e5de4) into develop (751b628) will decrease coverage by 1.82%.
The diff coverage is 57.50%.

@@             Coverage Diff             @@
##           develop     #145      +/-   ##
===========================================
- Coverage    78.07%   76.25%   -1.82%     
===========================================
  Files           32       34       +2     
  Lines         2915     3155     +240     
  Branches       690      764      +74     
===========================================
+ Hits          2276     2406     +130     
- Misses         461      552      +91     
- Partials       178      197      +19

Impacted Files	Coverage Δ
petab/lint.py	`72.20% <14.28%> (-2.35%)`	⬇️
petab/problem.py	`64.51% <27.27%> (-2.79%)`	⬇️
petab/parameters.py	`84.53% <57.14%> (-2.68%)`	⬇️
petab/mapping.py	`57.89% <57.89%> (ø)`
petab/observables.py	`97.26% <60.00%> (-2.74%)`	⬇️
petab/models/pysb_model.py	`60.80% <60.80%> (ø)`
petab/models/model.py	`68.33% <64.28%> (-3.67%)`	⬇️
petab/models/sbml_model.py	`88.40% <66.66%> (-7.96%)`	⬇️
petab/yaml.py	`85.88% <66.66%> (-1.93%)`	⬇️
petab/C.py	`100.00% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

petab/problem.py

dilpath

Looks good 👍 Did not properly check the pysb_model.py or test_model_pysb.py files, but code quality is good. There are some TODOs left in the code

petab/C.py

petab/lint.py

dilpath · 2023-03-01T00:42:48Z

petab/lint.py

+            # mapping table entities mapping to already allowed parameters
+            if to_id in allowed_in_condition_cols
+            # mapping table entities mapping to species
+            or model.is_state_variable(to_id)


Should these state variables be provided by model.get_valid_ids_for_condition_table() instead?

You are right. Needs to be changed.

Hm. The problem here is that the list of allowed species could be very long (potentially infinite) for rule-based models. But it doesn't make sense to not include them in model.get_valid_ids_for_condition_table.

Okay, no, it makes sense the way it is. For SBML, to_id in allowed_in_condition_cols will always do the job. For PySB, the species ID itself is not allowed in the condition table, because they don't match our PEtab identifier requirements. That's why we have the mapping table. For this case, we need the model.is_state_variable(to_id) part.

petab/mapping.py

dilpath · 2023-03-01T00:48:19Z

petab/models/__init__.py

 MODEL_TYPE_SBML = 'sbml'
+MODEL_TYPE_PYSB = 'pysb'

-known_model_types = {MODEL_TYPE_SBML}
+known_model_types = {
+    MODEL_TYPE_SBML,
+    MODEL_TYPE_PYSB,
+}


Could use ModelType.SBML/PYSB enum

Would keep that for later.

dilpath · 2023-03-01T00:49:19Z

petab/models/model.py

    :returns: A :py:class:`Model` instance representing the given model
    """
+    from . import MODEL_TYPE_SBML, MODEL_TYPE_PYSB, known_model_types


Move to header?

Would cause circular import issues. petab/models/__init__.py imports from petab/models/model.py

petab/models/sbml_model.py

dilpath · 2023-03-01T01:08:33Z

petab/yaml.py

+        mapping_file = get_rel_to_yaml([mapping_file])[0]\
+            if mapping_file else None


Suggested change

mapping_file = get_rel_to_yaml([mapping_file])[0]\

if mapping_file else None

mapping_file = get_rel_to_yaml(mapping_file)

Right, so far it's only a single file name.

Ah, now I'm confused by my suggestion, because get_rel_to_yaml expects List[str]...

And it makes more sense to change it to a list of filenames for coherence with the other file types.

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

FFroehlich

👍

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

dweindl force-pushed the model_pysb branch 2 times, most recently from 045c364 to 3fd9d55 Compare May 24, 2022 15:11

Base automatically changed from abstract_model to develop June 22, 2022 15:01

dweindl force-pushed the model_pysb branch from 66c56d0 to 3650362 Compare June 24, 2022 21:43

dweindl force-pushed the model_pysb branch from 815ef03 to b3a2e0c Compare July 12, 2022 13:15

dweindl added 5 commits July 19, 2022 22:41

Add PySBModel for handling of PySB models

49fe8e4

Use tentative multilanguage schema

b591058

Add mapping table IO

b74a867

Condition table validation respects mapping table

9c0135b

Add PySB ID parsing and checking

8e74e25

dweindl force-pushed the model_pysb branch from cc5ce2e to 8e74e25 Compare July 19, 2022 20:41

dweindl mentioned this pull request Jul 22, 2022

PEtab-multilanguage proof of concept for PySB-PEtab AMICI-dev/AMICI#1800

Merged

dweindl and others added 4 commits July 25, 2022 10:02

Merge branch 'develop' into model_pysb

bb20d7c

circ

f537d28

Merge branch 'develop' into model_pysb

b35e628

Merge branch 'develop' into model_pysb

c1b356d

dilpath reviewed Oct 16, 2022

View reviewed changes

petab/problem.py Outdated Show resolved Hide resolved

dweindl and others added 8 commits December 6, 2022 19:55

Merge branch 'develop' into model_pysb

bf08fdc

Merge branch 'develop' into model_pysb

b5b40cf

add support for mapping table when constructing parameter mapping

de3a23d

pattern_from_string

3385913

More mapping table handling

b3b0788

more mapping table handling

cee9ff0

Merge branch 'develop' into model_pysb

bb7b5a9

..

fde0617

dweindl marked this pull request as ready for review February 28, 2023 18:12

dweindl requested a review from a team as a code owner February 28, 2023 18:13

update url

7063b3c

dilpath approved these changes Mar 1, 2023

View reviewed changes

dweindl and others added 7 commits March 8, 2023 15:22

Update petab/yaml.py

70e1e1e

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

msg

3e452ab

reset_index

f47e745

Update petab/mapping.py

17359b2

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

all rule targets

cbf91f7

Merge remote-tracking branch 'origin/develop' into model_pysb

ada7b98

todo

1986558

dweindl requested a review from FFroehlich March 8, 2023 15:15

multiple mapping files

91e5de4

FFroehlich approved these changes Mar 9, 2023

View reviewed changes

dweindl merged commit 92ad3b9 into develop Mar 9, 2023

dweindl deleted the model_pysb branch March 9, 2023 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PySBModel for handling of PySB models #145

Add PySBModel for handling of PySB models #145

dweindl commented May 4, 2022 •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading

dilpath left a comment

dilpath Mar 1, 2023

dweindl Mar 8, 2023

dweindl Mar 8, 2023

dweindl Mar 8, 2023

dilpath Mar 1, 2023

dweindl Mar 8, 2023

dilpath Mar 1, 2023

dweindl Mar 8, 2023

dilpath Mar 1, 2023

dweindl Mar 8, 2023

dilpath Mar 9, 2023

dweindl Mar 9, 2023

FFroehlich left a comment

		mapping_file = get_rel_to_yaml([mapping_file])[0]\
		if mapping_file else None

	mapping_file = get_rel_to_yaml([mapping_file])[0]\
	if mapping_file else None
	mapping_file = get_rel_to_yaml(mapping_file)

Add PySBModel for handling of PySB models #145

Add PySBModel for handling of PySB models #145

Conversation

dweindl commented May 4, 2022 • edited Loading

codecov-commenter commented May 24, 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

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

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

dweindl commented May 4, 2022 •

edited

Loading

codecov-commenter commented May 24, 2022 •

edited

Loading