update flepimop2 provider pacakge by MacdonaldJoshuaCaleb · Pull Request #12 · ACCIDDA/op_engine

MacdonaldJoshuaCaleb · 2026-02-13T12:33:27Z

This pull request consolidates and refactors the integration between flepimop2 and op_engine, moving all engine logic and configuration into a single file (__init__.py). It removes the separate config.py and engine.py modules, simplifies dependency management in pyproject.toml, and updates linting rules. The new implementation provides a streamlined, self-contained flepimop2 engine backed by op_engine, with improved configuration validation and runtime checks.

Engine integration and refactoring:

Merged all engine logic and configuration into src/flepimop2/engine/op_engine/__init__.py, replacing the previous split between engine.py and config.py. The new implementation defines the OpEngineFlepimop2Engine and its configuration in a single file, with improved runtime validation and operator handling.

Dependency and build configuration:

Updated pyproject.toml to reference op-engine directly from its GitHub repository, removing the sibling directory wiring and simplifying dependency management.
Linting and code quality:
Added per-file ignore rules in pyproject.toml for the new consolidated __init__.py, disabling specific lint and docstyle checks to accommodate the new implementation.
Aligned provider package with the expectations setup in flepimop2 #118

Closes #6 obsoletes #10, #11 due to changes introduced in flepimop2 #118

TimothyWillard

It would nice to see a bit more DRY, especially for low hanging fruit like using types from flepimop2 or helpers already defined in op_engine.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-17T16:28:23Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    dt_min: float = Field(default=0.0, ge=0.0)
+    dt_max: float = Field(default=float("inf"), gt=0.0)


Does dt_min == 0.0 mean infer lower bound and dt_max == math.inf mean infer upper bound? Minor, but seems like a more natural choice would be None since that indicates no user statement.

I kept dt_min=0.0 and dt_max=inf intentionally because those are the native op_engine DtControllerConfig sentinel values for ‘no additional bound’. Switching to None in the provider would require extra normalization logic and would diverge from core config semantics without changing behavior. If we want None ergonomics, we should introduce it consistently in op_engine itself first, then mirror it here.

Yeah, that seems reasonable to mirror the API of op_engine here. I guess just noting its a bit weird to use concrete values rather than values that indicate "no statement", but I guess this is outside the scope of this PR then.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-17T16:38:20Z

flepimop2-op_engine/tests/data/sir.py

This is not a TODO for you @MacdonaldJoshuaCaleb instead for me, but probably need to provide some kind of default system/engine as part of the integration testing utilities bundled with flepimop2.

flepimop2-op_engine/tests/test_engine.py

pearsonca

Minor questions, but seems fine?

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

pearsonca · 2026-02-17T19:16:15Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    return None
+
+
+def _rhs_from_stepper(


work for another day: if adapter is necessary, we should probably be working the pipeline definition a bit. i imagine generally engines may need to adapt SystemABC somewhat, but all this re-shaping seems problematic.

This actually seems fine to me, and within the scope of engines. This is taking a stepper function and then vectorizing it because that's the type of "stepper" that op_engine expects. I don't think flepimop2 should get into the business of trying to figure out the correct way(s) to generalize steppers. However, I do think this could be simplified a bit by the introduction of proper "build" mechanics, see ACCIDDA/flepimop2#130, ACCIDDA/flepimop2#129 (comment).

I'm mostly bothered by the shaping related checks than the reshaping itself. I should have said if this amount of adaptor required

I see, I think this is a consequence of the outputs being unstructured numpy arrays. To address this would require adding more structure on that front. Perhaps ACCIDDA/flepimop2#147 is a first step to that, but definitely more work required beyond that.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

MacdonaldJoshuaCaleb

Why not use op_engine.types.as_float_1d?

as part of the simplifications I got rid of the types.py file

TimothyWillard · 2026-02-18T13:50:13Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

 from flepimop2.exceptions import ValidationIssue
-from flepimop2.system.abc import SystemABC, SystemProtocol
-from pydantic import BaseModel, ConfigDict, Field, model_validator
+from flepimop2.typing import StateChangeEnum  # noqa: TC002


Instead of noqaing, why not move this under the type checking imports as the lint rule suggests? See https://docs.astral.sh/ruff/rules/typing-only-third-party-import/.

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

TimothyWillard · 2026-02-18T14:03:14Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    return None
+
+
+def _rhs_from_stepper(


This actually seems fine to me, and within the scope of engines. This is taking a stepper function and then vectorizing it because that's the type of "stepper" that op_engine expects. I don't think flepimop2 should get into the business of trying to figure out the correct way(s) to generalize steppers. However, I do think this could be simplified a bit by the introduction of proper "build" mechanics, see ACCIDDA/flepimop2#130, ACCIDDA/flepimop2#129 (comment).

TimothyWillard · 2026-02-18T14:06:39Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    dt_min: float = Field(default=0.0, ge=0.0)
+    dt_max: float = Field(default=float("inf"), gt=0.0)


Yeah, that seems reasonable to mirror the API of op_engine here. I guess just noting its a bit weird to use concrete values rather than values that indicate "no statement", but I guess this is outside the scope of this PR then.

TimothyWillard · 2026-02-18T14:14:57Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+    """flepimop2 engine adapter backed by op_engine.CoreSolver."""
+
+    module: Literal["flepimop2.engine.op_engine"] = "flepimop2.engine.op_engine"
+    state_change: StateChangeEnum


Okay, I'm a bit confused about this one. It seems like this seems to indicate that op_engine handles multiple different kinds of state change steppers, but requires user configuration to do so? If op_engine can handle multiple kinds of state change steppers it should just use system.state_change to infer the settings for op_engine. However, it appears like a validation is done, and then self.state_change is never used? So can op_engine actually handle steppers of different state changes, from your description of the package I thought it could only handle "flow"?

TimothyWillard · 2026-02-18T14:20:46Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/__init__.py

+            if isinstance(system_axis, str | int):
+                operator_axis = system_axis
+
+        stepper: SystemProtocol = system._stepper  # noqa: SLF001


Another argument for pushing ACCIDDA/flepimop2#130 forward in the workplan

TimothyWillard · 2026-02-18T14:23:20Z

flepimop2-op_engine/src/flepimop2/engine/op_engine/config.py

-            tr=operators.get("tr"),
-            bdf2=operators.get("bdf2"),
-        )
+__all__ = ["OpEngineEngineConfig", "_coerce_operator_specs", "_has_operator_specs"]


Why export _coerce_operator_specs & _has_operator_specs? These seem like private utilities.

TimothyWillard · 2026-02-18T14:27:24Z

flepimop2-op_engine/pyproject.toml


 [tool.ruff.lint.per-file-ignores]
 "tests/**/*" = ["INP001", "S101"]
+"src/flepimop2/engine/op_engine/__init__.py" = ["C901", "DOC201", "DOC501", "PLR0912", "PLR0913", "PLR0914", "PLR0915", "RUF067"]


This makes me a bit weary to ignore so many linting/formatting suggestions. Perhaps outside the scope of this PR, but I think these rules exist to indicate where there might be refactoring/documentation opportunities. Although, for RUF067 we also need to think about that on the flepimop2 front, the way we've structured the package as being an implicit namespace package with dynamic module loading makes it a little annoying to work around this one.

reduce introspection in flepimop2 provider package

4d3c081

MacdonaldJoshuaCaleb self-assigned this Feb 13, 2026

MacdonaldJoshuaCaleb requested review from TimothyWillard and pearsonca February 13, 2026 12:33

MacdonaldJoshuaCaleb and others added 3 commits February 13, 2026 07:36

remove extra line in pyproject.toml

c30bdb5

Merge branch 'main' into reduce_introspection

1d38f47

Simplify flepimop2 provider and align contracts

bd49d88

MacdonaldJoshuaCaleb changed the title ~~reduce introspection in flepimop2 provider package~~ update flepimop2 provider pacakge Feb 17, 2026

TimothyWillard reviewed Feb 17, 2026

View reviewed changes

pearsonca reviewed Feb 17, 2026

View reviewed changes

MacdonaldJoshuaCaleb commented Feb 17, 2026

View reviewed changes

MacdonaldJoshuaCaleb added 2 commits February 17, 2026 15:54

Align flepimop2 provider with latest module APIs

d1096cc

Use direct system.option lookups in op_engine provider

b0e8ec9

MacdonaldJoshuaCaleb requested review from TimothyWillard and pearsonca February 17, 2026 21:09

TimothyWillard mentioned this pull request Feb 18, 2026

Add example engine/system for integration testing purposes ACCIDDA/flepimop2#150

Open

TimothyWillard reviewed Feb 18, 2026

View reviewed changes

		dt_min: float = Field(default=0.0, ge=0.0)
		dt_max: float = Field(default=float("inf"), gt=0.0)

Conversation

MacdonaldJoshuaCaleb commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyWillard left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pearsonca left a comment

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!

MacdonaldJoshuaCaleb left a comment

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!

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!

3 participants

MacdonaldJoshuaCaleb commented Feb 13, 2026 •

edited

Loading