Support PEtab SciML problems and enable linting by m-philipps · Pull Request #482 · PEtab-dev/libpetab-python

m-philipps · 2026-05-26T22:26:39Z

WIP

codecov-commenter · 2026-06-11T15:02:14Z

Codecov Report

❌ Patch coverage is 68.95161% with 77 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.21%. Comparing base (ae4ea94) to head (87d273c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
petab/v2/base.py	76.10%	18 Missing and 9 partials ⚠️
petab/v2/core.py	60.00%	19 Missing and 7 partials ⚠️
petab/v2/sciml/core.py	66.03%	16 Missing and 2 partials ⚠️
petab/v2/sciml/lint.py	62.50%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
- Coverage   75.29%   75.21%   -0.08%     
==========================================
  Files          62       64       +2     
  Lines        6946     6755     -191     
  Branches     1229     1153      -76     
==========================================
- Hits         5230     5081     -149     
+ Misses       1245     1218      -27     
+ Partials      471      456      -15

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dilpath

Thanks! Overall it would be good to try to separate as much of the SciML things into separate files or subdirectory if possible. This will help later on when we design a nicer extension interface for libpetab-python, I think. It will also help with maintenance immediately.

dilpath · 2026-06-16T12:19:50Z

        return sum(p.estimate for p in self.parameters)


+class Hybridization(BaseModel):


Move SciML-specific things to a separate file or subdirectory e.g. sciml/core.py and import them here? Just for organization; might help to keep SciML things as separate as possible. e.g. do not import petab_sciml here, rather only in the separated file.

Nevermind this if it will lead to circular imports etc.

I've split out the sciml specific classes and introduced petab/v2/base.py to resolve circular imports. Let me know what you think.

There is still a fair amount of sciml related code left in petab/v2/core.py because the sciml fields have been added to the Problem class. We could consider an inheritance pattern in the future i.e.

class SciMLProblem(Problem): ...

IMO that would be worth considering if/when we want to support another PEtab extension that requires more fields in Problem.

Thanks a lot, that was very helpful. And I agree, no need to fully implement now.

dilpath · 2026-06-16T12:24:14Z

        config: ProblemConfig = None,
    ):
-        from ..v2.lint import default_validation_tasks
+        from ..v2.lint import default_validation_tasks, sciml_validation_tasks


Also this, import from ..v2.sciml.lint for example, for separation.

dilpath · 2026-06-16T12:37:08Z

+            for mapping in problem.mappings:
+                # petabEntityId is not referenced in any model
+                for model in problem.models:
+                    if model.has_entity_with_id(mapping.petab_id):
+                        messages.append(
+                            f"`{mapping.petab_id}` is used in the mapping "
+                            "table and referenced directly in the model "
+                            f"`{model.model_id}`."
+                        )


The spec currently says that the petabEntityId can be the same as a modelEntityId, hence the petabEntityIds can appear in the model, right? @dweindl what do you think?

The petabEntityId may be the same as the modelEntityId, but it must not be used to alias an entity that already has a valid PEtab identifier. This restriction is to avoid unnecessary complexity in the PEtab problem files.

There is some discussion of this here PEtab-dev/PEtab#669

I added another clause to the conditional here and below to allow the case where petabEntityId == modelEntityId. Please double check that I've understood the nuance correctly.

Thanks, looks good

dilpath · 2026-06-16T12:42:37Z

+    # Add petab ids from mapping table if they are used for aliasing
    for mapping in problem.mappings:
-        if mapping.model_id and mapping.model_id in parameter_ids.keys():
+        if mapping.petab_id not in invalid:
            parameter_ids[mapping.petab_id] = None
+        # An aliased model id is not a valid parameter id
+        if mapping.model_id and mapping.model_id in parameter_ids:
+            del parameter_ids[mapping.model_id]


As above, I think it's OK to map a model ID to a PEtab ID in the mapping table as long as it's not an alias, i.e. only in the special case where petabEntityId == modelEntityId. I think the reason for this in the spec is for support use of the mapping table for annotation, i.e. via additional user-defined annotation columns.

As above, if we allow using the mapping table for general annotation of petab entities, which I think we do, then the current implementation would incorrectly consider, e.g., observables to be allowed in the parameter table, wouldn't it?

dilpath

Thanks very much! I would be happy to get @dweindl 's approval before merging, since it touches his v2 code a bit.

Currently I don't think CheckHybridizationTable is properly tested, but fine for now since it's just a placeholder before we implement all SciML checks.

dilpath · 2026-06-17T15:18:05Z

+    @property
+    def hybridizations(self) -> list[Hybridization]:
+        """List of hybridizations in the hybridization table(s)."""
+        return list(
+            chain.from_iterable(
+                ht.hybridizations for ht in self.hybridization_tables
+            )
+        )
+
+    @property
+    def hybridization_df(self) -> pd.DataFrame | None:
+        """Combined SciML hybridization tables as DataFrame."""
+        return (
+            HybridizationTable(hybridizations).to_df()
+            if (hybridizations := self.hybridizations)
+            else None
+        )
+
+    @hybridization_df.setter
+    def hybridization_df(self, value: pd.DataFrame):
+        self.hybridization_tables = [HybridizationTable.from_df(value)]


Add some "Specific to PEtab SciML." comment to all docstrings of methods that are implemented in core PEtab?

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

dweindl · 2026-06-17T19:23:40Z

I'd prefer moving all extension subpackages to .ext.{extension_id} to avoid collisions with other petab modules.

dweindl · 2026-06-17T19:28:50Z

+
+import petab.v2.C as C
+
+from ...v1.math.sympify import sympify_petab


Suggested change

from ...v1.math.sympify import sympify_petab

from ..math.sympify import sympify_petab

Currently this will import the same, but it's safer in case we separate v1/v2 math processing.

dweindl · 2026-06-17T19:41:13Z

@@ -0,0 +1,225 @@
+"""Base classes shared across petab.v2 to avoid circular imports."""


Hm... .core was meant for that already.

dweindl · 2026-06-17T19:44:47Z

 #: Extensions key in the YAML file
 EXTENSIONS = "extensions"
+#: PEtab SciML extension
+SCIML = "sciml"


Suggested change

SCIML = "sciml"

EXT_ID_SCIML = "sciml"

Feels safer. We might have extension IDs that collide with existing constants here.

dweindl · 2026-06-17T19:54:21Z

            else None
        )

+        neural_networks = None


I am not super happy with adding all that to Problem. I see some of the changes are to invasive to fully isolate them, but for many things it should be possible.
How about having some Problem.ext.sciml: petab.v2.ext.sciml.SciMlExt, loading all those things in SciMlExt.__init__ and accessing all those entities through problem.ext.sciml.{hybridizations,neural_networks,...}? Too verbose?
This would also circumvent the circular import issues. Those extension classes would only have to imported at the function level, not module level, and we wouldn't have to move things to base.py.

dweindl · 2026-06-17T20:14:44Z

+                        model.has_entity_with_id(mapping.petab_id)
+                        and mapping.petab_id != mapping.model_id


Swap conditions - the second one is way cheaper to evaluate.

dweindl · 2026-06-17T20:25:26Z

+    # Add petab ids from mapping table if they are used for aliasing
    for mapping in problem.mappings:
-        if mapping.model_id and mapping.model_id in parameter_ids.keys():
+        if mapping.petab_id not in invalid:
            parameter_ids[mapping.petab_id] = None
+        # An aliased model id is not a valid parameter id
+        if mapping.model_id and mapping.model_id in parameter_ids:
+            del parameter_ids[mapping.model_id]


As above, if we allow using the mapping table for general annotation of petab entities, which I think we do, then the current implementation would incorrectly consider, e.g., observables to be allowed in the parameter table, wouldn't it?

dweindl · 2026-06-17T20:36:39Z

+def test_check_mapping_table():
+    """Test checks related to the mapping table."""
+    problem = Problem()
+    # PySB model from PEtab test suite


Suggested change

# PySB model from PEtab test suite

# SBML model from PEtab test suite

and not really from the test suite?

dweindl · 2026-06-17T20:44:02Z

+    """Test checks related to the mapping table."""
+    problem = Problem()
+    # PySB model from PEtab test suite
+    problem.model = SbmlModel.from_antimony("a.mean = 1")


This doesn't create a parameter with ID a.mean, but creates an sbml-distrib construct. Is this intended? If so, please describe the rationale in more detail.
If it's just about creating some model with a petab-incompatible ID, then it would have to be a PySB model. In SBML, we don't have that issue and the mapping table will only ever be used for annotation.

dweindl · 2026-06-17T20:48:08Z

    "scipy"
 ]
+sciml = [
+    "petab_sciml @ git+https://github.com/PEtab-dev/petab_sciml.git",


Both git/github references added here will break PyPI deployment. petab_sciml will have to be installed manually until we can reference a pypi package here.

m-philipps changed the title ~~Support a PEtab SciML problem and enable linting~~ Support PEtab SciML problems and enable linting May 26, 2026

m-philipps and others added 20 commits June 11, 2026 15:38

update data types

3d552a9

add checks for mapping table

d003839

Use SBML in test

c3e8819

ensure that all observables have a noise_formula

f4278c9

rollback

0a54558

add hybridization class

db6fbd6

add sciml problem config

5f89cab

create Problem for sciml extension

2fbc20e

Assign SciML-specific lint checks

1c4330c

remove lint checks that are covered by pydantic

648a21a

Proble.add_hybridization method

be625d6

add hybridization df property, setter

32c4b4b

fix

0de7a5f

add a basic test for sciml

5499ee1

methods for adding nn, array data to problem

64b8c25

Non-local petab_sciml import

e426cb7

add dependency

8227ce0

Update petab_sciml dependency to use Git URL

0123281

update allowed params logic for mappings

317b810

neural_nets to neural_networks

cb5cef1

BSnelling force-pushed the sciml_lint branch from 3295eec to cb5cef1 Compare June 11, 2026 14:57

BSnelling added 2 commits June 15, 2026 10:38

add required params check to sciml validations

4a208f9

ruff format

111658e

BSnelling marked this pull request as ready for review June 16, 2026 10:41

BSnelling requested a review from a team as a code owner June 16, 2026 10:41

dilpath requested changes Jun 16, 2026

View reviewed changes

BSnelling added 2 commits June 17, 2026 11:43

implement feedback from code review

7050ff9

move sciml code to separate files and resolve circular imports

153289f

BSnelling added 2 commits June 17, 2026 14:48

fixup ruff

7bb786e

fix docs build issue

2972df0

BSnelling requested a review from dweindl as a code owner June 17, 2026 14:23

BSnelling requested a review from dilpath June 17, 2026 14:33

dilpath approved these changes Jun 17, 2026

View reviewed changes

BSnelling and others added 2 commits June 17, 2026 16:26

Update petab/v2/core.py

8a138ea

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

add to docstrings

87d273c

dweindl requested changes Jun 17, 2026

View reviewed changes

		return sum(p.estimate for p in self.parameters)


		class Hybridization(BaseModel):


		import petab.v2.C as C

		from ...v1.math.sympify import sympify_petab

	from ...v1.math.sympify import sympify_petab
	from ..math.sympify import sympify_petab

		@@ -0,0 +1,225 @@
		"""Base classes shared across petab.v2 to avoid circular imports."""

		model.has_entity_with_id(mapping.petab_id)
		and mapping.petab_id != mapping.model_id

	# PySB model from PEtab test suite
	# SBML model from PEtab test suite

Conversation

m-philipps commented May 26, 2026

Uh oh!

codecov-commenter commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dilpath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilpath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 11, 2026 •

edited

Loading