Review by davidhjp01 · Pull Request #1 · dnv-opensource/PythonFMU3

davidhjp01 · 2026-10-22T07:56:24Z

Just created a PR to leave review comments
@eisDNV

…oving tests/ out. Ran ruff check --fix and ruff format on project

… ============================== platform linux -- Python 3.13.12, pytest-9.1.1, pluggy-1.6.0 rootdir: /home/se/DNV/osp/packages/PythonFMU3 configfile: pyproject.toml plugins: cov-7.1.0 collected 1867 items / 1 skipped tests/test_builder.py ........... [ 0%] tests/test_deploy.py ........................ [ 1%] tests/test_docs.py ... [ 2%] tests/test_fmi3slave.py ................................................ [ 4%] [ 4%] tests/test_integration.py sssssssssssssss [ 5%] tests/test_multiple_fmus.py s [ 5%] tests/test_variables.py ................................................ [ 8%] ........................................................................ [ 11%] ........................................................................ [ 15%] ........................................................................ [ 19%] ........................................................................ [ 23%] ........................................................................ [ 27%] ........................................................................ [ 31%] ........................................................................ [ 35%] ........................................................................ [ 38%] ........................................................................ [ 42%] ........................................................................ [ 46%] ........................................................................ [ 50%] ........................................................................ [ 54%] ........................................................................ [ 58%] ........................................................................ [ 62%] ........................................................................ [ 65%] ........................................................................ [ 69%] ........................................................................ [ 73%] ........................................................................ [ 77%] ........................................................................ [ 81%] ........................................................................ [ 85%] ........................................................................ [ 89%] ........................................................................ [ 92%] ........................................................................ [ 96%] ............................................................. [100%] ======================= 1851 passed, 17 skipped in 1.96s ======================= passes all 1851 tests

…therefore docs/usage.md and adapted the documentation build process (adding conf.py, /_static and /_templates)

…ode. Successfull compilation. Added a basic discrete event example.

…s is not only in use for ME, but also for CS in event mode. It is set to True per default as per FMI spec

…tion also to other variable types and added ModelVariables.to_str as formatter for python types to xml strings

…de compiles, but is so far not yet tested.

…that no switch back to step mode was possible. Compilation ok.

…amedTuple needs to be defined and used.

…features. Options are now implemented as dataclass and it is possible to provide options directly to the Slave (not only through cli). Tested features.

…n FMU building and FMU usage with respect to variables with dimensions

…rd. SetupExperiment deprecated, EnterInitializationMode updated, EnterContinuousTimeMode and CompletedIntegratorStep added. Recompiled and basic tests.

…ntiateCosimulation, which where so far not included. Related changes in fmi3slave are also implemented. Compiles and test works. IntermediateUpdate management is still not included.

…ion not yet included.

…enerated FMUs have been added to the git package (.gitignore changed).

davidhjp01

Hi Siegfried, here are my reviews.
I did not mention any typing issues or commented code, which I think can be addressed separately.

davidhjp01 · 2026-10-22T08:18:15Z

+            )
+        )
+
+    def do_step(self, current_time, step_size):


Missing positional arg '*' -

PythonFMU3/src/pythonfmu3/pythonfmu-export/src/pythonfmu/PySlaveInstance.cpp

Line 245 in 44bada4

auto f = PyObject_CallMethod(pInstance_, "do_step", "(ddi)", currentTime, stepSize, noSetFmuStatePriorToCurrentPoint);

I agree. It would probably be good to define a layer where this additional argument receives a default value value, so that it becomes optional

davidhjp01 · 2026-10-22T08:21:13Z

+            terminateSimulation,
+            earlyReturn,
+            lastSuccessfullTime);
+        return fmi3OK;


I guess we need to return appropriate return status depending on the result of slave->DoStep?

davidhjp01 · 2026-10-22T08:26:54Z

+            me_option_names = list(Fmi3ModelOptionsME().to_dict().keys())
+            if type_node is not None and 'options' in options:
+                for option, value in options['options'].items():
                    if option in mx_option_names:


Not from the PR, but need to be me_option_names instead?

davidhjp01 · 2026-10-22T08:42:40Z

        if (PyObject_HasAttrString(f, "terminateSimulation")) {
            PyObject* pyTerminateSimulation = PyObject_GetAttrString(f, "terminateSimulation");
            if (pyTerminateSimulation) {
                *terminateSimulation = static_cast<bool>(PyObject_IsTrue(pyTerminateSimulation));
                Py_DECREF(pyTerminateSimulation);
            }
        }


Also not from PR, but terminateSimulation needs to be assigned if attr is missing

*assign a default value

davidhjp01 · 2026-10-22T08:42:46Z

+        } else {
+            *earlyReturn = false;
+        }
+        if (PyObject_HasAttrString(f, "lastSuccessfullTime")) {


davidhjp01 · 2026-10-22T10:14:11Z

+            PyObject* value = PyList_GetItem(ref0, i);
+            intervals[i] = PyFloat_AsDouble(value);
+        }
+        Py_DECREF(ref0);


These PyList_GetItem and other getters that return pointers do not owned by the caller, so should not be destroyed

I think we need to Py_INCREF before DECREF

davidhjp01 · 2026-10-22T10:15:52Z

+    then indirect the call to the Python executable, and let Python run the CLI script
+    as a module. This allows the debugger to follow into the subprocess.
+    """
+    CLI_ENTRY_POINT = "sim-explorer"


We should not use external app specific test?

Not quite sure what you mean with that?

davidhjp01 · 2026-10-22T10:16:58Z

+    eventHandlingNeeded: bool = False
+    terminateSimulation: bool = False
+    earlyReturn: bool = False
+    lastSuccessfullTime: "Float64" = 0.0


davidhjp01 · 2026-10-22T10:18:06Z

+class Fmi3StepResult(NamedTuple):
+    eventHandlingNeeded: bool = False
+    terminateSimulation: bool = False
+    earlyReturn: bool = False
+    lastSuccessfullTime: "Float64" = 0.0


Duplicated definition

davidhjp01 · 2026-10-22T10:19:20Z

 import platform
 import tempfile
 import zipfile
+from attrs import asdict


this dep should be declared as test depndencies (if only used in test)

…used and early_return_allowed

eisDNV and others added 18 commits August 4, 2026 10:21

Re-structured project to the standard src/pythonfmu3/ structure and m…

e0f9f67

…oving tests/ out. Ran ruff check --fix and ruff format on project

Added the PID-controller example to /examples. Needed fixes. Changed …

c042b82

…therefore docs/usage.md and adapted the documentation build process (adding conf.py, /_static and /_templates)

Added the implementation of EnterEventMode fmi3 function to the Cpp c…

914b037

…ode. Successfull compilation. Added a basic discrete event example.

Updated the documentation of ModelVariables class and added since thi…

4e078bb

…s is not only in use for ME, but also for CS in event mode. It is set to True per default as per FMI spec

Added a variable type 'Clock' to ModelVariables. Added some documenta…

65ab30d

…tion also to other variable types and added ModelVariables.to_str as formatter for python types to xml strings

Implemented all event-related functions in cppfmu/ and pythonfmu/. Co…

c3673ee

…de compiles, but is so far not yet tested.

Added the function EnterStepMode, which was completely missing, such …

ae48cae

…that no switch back to step mode was possible. Compilation ok.

In order to receive results from UpdateDiscreteStates() a dedicated N…

200a59e

…amedTuple needs to be defined and used.

Ensure all FMI3 model options, so that the system can work with FMI3 …

e78660d

…features. Options are now implemented as dataclass and it is possible to provide options directly to the Slave (not only through cli). Tested features.

Change in builder.py to include model options properly

bfc7144

Corrected errors in fmi3slave.py and variables.py leading to errors i…

7432e2d

…n FMU building and FMU usage with respect to variables with dimensions

Added an FMI3 FMU 'function'. Tested basic usage.

bdc012a

Adapted function signatures to be as close as possible to FMI3 standa…

1f96296

…rd. SetupExperiment deprecated, EnterInitializationMode updated, EnterContinuousTimeMode and CompletedIntegratorStep added. Recompiled and basic tests.

Added the arguments 'eventModeUsed' and 'earlyReturnAllowed' in Insta…

8581c7c

…ntiateCosimulation, which where so far not included. Related changes in fmi3slave are also implemented. Compiles and test works. IntermediateUpdate management is still not included.

Some last changes as part of moving back to windows. Windows compilat…

fd4ab83

…ion not yet included.

Added windows support to the package. The dynamic libraries and the g…

5075170

…enerated FMUs have been added to the git package (.gitignore changed).

Minor updates within the example FMUs to support discrete_event mode

44bada4

davidhjp01 commented Oct 22, 2026

View reviewed changes

eisDNV added 2 commits October 26, 2026 08:45

Added CHANGELOG.md to the package

77dec22

Fixed issues detected in verification + Python syntax for event_mode_…

d3fc9a3

…used and early_return_allowed

Conversation

davidhjp01 commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidhjp01 left a comment

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!

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

davidhjp01 commented Oct 22, 2026 •

edited

Loading