fix(pybind): register bound types before the signatures that reference them - #654
leonardocarreras wants to merge 1 commit into
Conversation
65539c1 to
2f5c66c
Compare
There was a problem hiding this comment.
DPsim LLM review
Claim vs. code: matches the description.
Found 13 medium (0 anchored to lines below).
🔵 Optional / low-confidence (13)
- Missing documentation for new base class bindings
[medium · 35% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:43 - Add documentation page for TurbineGovernorType1 signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/TurbineGovernorType1.md - Add documentation page for GovernorParameters signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/GovernorParameters.md - Add documentation page for Governor signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/Governor.md - Add documentation page for TurbineParameters signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/TurbineParameters.md - Add documentation page for Turbine signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/Turbine.md - Add documentation page for PSSParameters signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/PSSParameters.md - Add documentation page for PSS signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/PSS.md - Missing test for addSignalComponentBases ordering
[medium · 35% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:272 - Missing documentation for newly bound PSS base class
[medium · 30% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:43 - Missing documentation for newly bound Governor/Turbine base classes
[medium · 30% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:61 - Missing documentation for newly bound signal base component classes
[medium · 30% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:30 - Missing test coverage for newly exposed signal base classes
[medium · 30% confidence · unconfirmed]indpsim/src/pybind/SignalComponents.cpp:36
Claim vs. implementation
- Claimed: Register bound pybind types before any signatures that reference them, and split signal base registrations to break a dependency cycle.
- Done: Reorders module/submodule and class registration in pybind main, predeclares several core types before attaching methods, and splits signal base class bindings into a new addSignalComponentBases() called before addSignalComponents().
- Difference: none
How this review was produced
13 specialized finder passes raised 56 findings over the diff and the full changed sources. After de-duplication, 56 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 43 refuted as unsupported, 13 kept (13 tentative).
Refuted by verification:
- Verify base class registration order for TurbineGovernorType1 (dpsim/src/pybind/SignalComponents.cpp): The file registers the base classes before the concrete TurbineGovernorType1 binding inside addSignalComponentBases, matching the intended pybind multiple-inheritance order.
- Missing override for mnaComp hooks in TurbineGovernorType1 (dpsim/src/pybind/SignalComponents.cpp): TurbineGovernorType1 is a signal component and the file only binds set_parameters/initialize_states; no mnaComp hooks are present or required here.
- Missing parameter validation in TurbineGovernorType1::setParameters (dpsim/src/pybind/SignalComponents.cpp): The binding simply forwards setParameters with named arguments; there is no validation logic in the binding to omit or add here.
- Missing state initialization guard in TurbineGovernorType1::initializeStates (dpsim/src/pybind/SignalComponents.cpp): The initialize_states binding directly forwards initializeStates with the TmRef argument; no guard is present in the file.
- Use DOUBLE_EPSILON for near-zero checks in governor/turbine parameter bindings (dpsim/src/pybind/SignalComponents.cpp): The file does not perform input validation or use DOUBLE_EPSILON in these bindings.
- Ensure Signal component bases are registered before Signal components to avoid dependency cycle in task graph (dpsim/src/pybind/main.cpp): The file explicitly calls addSignalComponentBases(mSignal) before addSignalComponents(mSignal), matching the intended composite registration pattern.
- Ensure all py::class_ handles remain alive until module definition completes (dpsim/src/pybind/main.cpp): The py::class_ handles are declared in the same PYBIND11_MODULE scope and remain live through the later .def() uses.
- Missing override specifier on virtual destructor for Base classes (dpsim/src/pybind/SignalComponents.cpp): The related base headers already declare virtual destructors for GovernorParameters, Governor, PSSParameters, PSS, TurbineParameters, and Turbine.
- TurbineGovernorType1 constructor exposes long positional parameter list (dpsim/src/pybind/SignalComponents.cpp): The constructor is intentionally exposed via py::init<std::string, CPS::Logger::Level>() in the binding.
- Unused base class bindings in addSignalComponentBases (dpsim/src/pybind/SignalComponents.cpp): TopologicalSignalComp and SimSignalComp are used as base classes in later py::class_ registrations throughout the same file.
- Rename function to reflect its purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases accurately describes that it registers base signal classes before the concrete bindings.
- Inconsistent submodule docstring (dpsim/src/pybind/main.cpp): The submodule is intentionally named with the docstring "signal models" at the m.def_submodule call.
- Remove Logger::Level constructor from TurbineGovernorType1 binding (dpsim/src/pybind/SignalComponents.cpp): The Logger::Level constructor is explicitly bound for TurbineGovernorType1, so it is not missing or removable on this evidence.
- Remove Logger::Level constructor from ExciterDC1 binding (dpsim/src/pybind/SignalComponents.cpp): ExciterDC1 also exposes a std::string, Logger::Level constructor in the same file.
- Remove Logger::Level constructor from ExciterDC1Simp binding (dpsim/src/pybind/SignalComponents.cpp): ExciterDC1Simp also exposes a std::string, Logger::Level constructor in the same file.
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
There was a problem hiding this comment.
DPsim LLM review
TL;DR: The only issues surfaced are four medium documentation gaps for newly exposed signal components; the binding-order fix itself did not produce any correctness, scheduling, or equation/stamping concerns in the review passes. Overall risk is low, but the new Python-facing types should have matching docs to avoid discoverability and usage problems.
Found 4 medium (0 anchored to lines below).
🔵 Optional / low-confidence (4)
- Add documentation page for TurbineGovernorType1 signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/TurbineGovernorType1.md - Add documentation page for PSS signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/PSS.md - Add documentation page for Governor signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/Governor.md - Add documentation page for Turbine signal component
[medium · 35% confidence · unconfirmed]indocs/hugo/content/en/docs/Models/Signal/Turbine.md
How this review was produced
13 specialized finder passes raised 47 findings over the diff and the full changed sources. After de-duplication, 47 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 43 refuted as unsupported, 4 kept (4 tentative).
Refuted by verification:
- Missing base class registration for PSSParameters and PSS in addSignalComponentBases (dpsim/src/pybind/SignalComponents.cpp): PSSParameters and PSS are explicitly registered in addSignalComponentBases at lines 43-47
- Add missing override for SimSignalComp in TurbineGovernorType1 bindings (dpsim/src/pybind/SignalComponents.cpp): TurbineGovernorType1 is bound with py::class_<..., CPS::SimSignalComp>(..., py::multiple_inheritance())
- Missing external tag for side-effecting task registration in addSignalComponentBases (dpsim/src/pybind/main.cpp): The file only reorders binding calls; there is no task registration or Scheduler dependency code here, and the composite binding pattern is not implicated.
- Order of addSignalComponents after addSignalComponentBases may still create dependency cycle in task graph (dpsim/src/pybind/main.cpp): The line simply calls addSignalComponents after addSignalComponentBases, matching the intended split and order in the file.
- Ensure Simulation class is fully defined before RealTimeSimulation binding (dpsim/src/pybind/main.cpp): Simulation is fully bound before the RealTimeSimulation block begins, and RealTimeSimulation is declared as deriving from Simulation in the binding.
- Use consistent py::class_ handle naming for base classes (dpsim/src/pybind/main.cpp): The base-class handles are declared and then used later in the file for method bindings and submodule setup.
- Add missing override specifiers to new virtual hooks in Base classes (dpsim/src/pybind/SignalComponents.cpp): The related base headers already declare these methods virtual/pure virtual, so no override declaration issue is shown here
- TurbineGovernorType1 exposes long positional parameter list in Python binding (dpsim/src/pybind/SignalComponents.cpp): This is a pybind argument list for an existing API, not a defect in the file
- Use named argument for logLevelArg in Simulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg is used consistently as the named default helper in the constructor binding, not as an unnamed positional argument.
- Use named argument for logLevelArg in RealTimeSimulation constructor binding (dpsim/src/pybind/main.cpp): RealTimeSimulation uses logLevelArg(CPS::Logger::Level::info) exactly at the constructor binding line.
- Use named argument for logLevelArg in CIMReader constructor binding (dpsim/src/pybind/main.cpp): CIMReader uses logLevelArg for both defaults, including the named "comploglevel" argument.
- Rename function to reflect its new purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases matches the implementation and the header refactor shown in the diff
- Add module docstring for submodule 'event' (dpsim/src/pybind/main.cpp): The event submodule is created with a docstring: m.def_submodule("event", "events").
- Use SPDLOG_LOGGER_DEBUG for debug-level logging in constructor (dpsim/src/pybind/SignalComponents.cpp): The constructor binding itself is present; the logging-style preference is not a code defect in this file
- Use SPDLOG_LOGGER_DEBUG for debug-level logging in ExciterDC1 constructor (dpsim/src/pybind/SignalComponents.cpp): The constructor binding itself is present; the logging-style preference is not a code defect in this file
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
There was a problem hiding this comment.
DPsim LLM review
No issues surfaced by the automated passes.
How this review was produced
13 specialized finder passes raised 27 findings over the diff and the full changed sources. After de-duplication, 27 were re-checked against the current file and the base-class / interface headers it inherits (code as truth), escalating survivors to a stronger model: 27 refuted as unsupported, 0 kept (0 tentative).
Refuted by verification:
- Verify base class registration order matches inheritance hierarchy (dpsim/src/pybind/SignalComponents.cpp): The file explicitly registers the base classes before the derived bindings in addSignalComponentBases, which is the intended pybind11 pattern.
- Ensure Signal component bases are registered before Signal components in Python bindings (dpsim/src/pybind/main.cpp): The file only reorders binding setup; it does not add any component implementation, and the claimed MNA dependency hooks are not something this binding file should declare.
- Missing explicit dependency on base signal classes in Python bindings (dpsim/src/pybind/main.cpp): The file explicitly registers the signal base classes via addSignalComponentBases(mSignal) before addSignalComponents(mSignal).
- Ensure Simulation class is fully defined before RealTimeSimulation inherits from it (dpsim/src/pybind/main.cpp): RealTimeSimulation is bound after Simulation in the same file, and the Simulation binding is already created at line 269 before the derived class declaration.
- SystemTopology class binding must follow all method definitions that reference it (dpsim/src/pybind/main.cpp): SystemTopology is declared as a py::class_ handle at line 267 before any Simulation methods use it, and its method bindings are intentionally attached later.
- Use consistent naming for py::class_ handles to avoid confusion (dpsim/src/pybind/main.cpp): The file consistently uses both named handles and direct temporary bindings; the absence of a handle for some classes is a style choice, not a defect.
- Missing override specifiers on virtual methods in SignalComponents.cpp (dpsim/src/pybind/SignalComponents.cpp): This is a pybind registration function, not a C++ virtual override site; no override specifier is applicable here.
- Multiple inheritance in pybind bindings without virtual inheritance (dpsim/src/pybind/SignalComponents.cpp): The binding already uses py::multiple_inheritance() exactly as written on the TurbineGovernorType1 class registration.
- Inconsistent default argument style in Simulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg(CPS::Logger::Level::off) is present in the Simulation constructor binding and is the intended replacement shown in the diff.
- Inconsistent default argument style in RealTimeSimulation constructor binding (dpsim/src/pybind/main.cpp): logLevelArg(CPS::Logger::Level::info) is present in the RealTimeSimulation constructor binding and is the intended replacement shown in the diff.
- Inconsistent default argument style in CIMReader constructor binding (dpsim/src/pybind/main.cpp): The CIMReader binding explicitly uses logLevelArg for both Logger::Level parameters on lines 559-560.
- rename function to match its purpose (dpsim/src/pybind/SignalComponents.cpp): The function name addSignalComponentBases is present in the source and matches its purpose of registering multiple base classes.
- inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): The constructor argument is named with "name"_a, and the log level uses the project’s helper-style default argument form.
- inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): The constructor argument is named with "name"_a, and the log level uses the project’s helper-style default argument form.
- inconsistent keyword argument naming (dpsim/src/pybind/main.cpp): Both CIMReader level arguments are given through logLevelArg, with the second explicitly named "comploglevel".
Automated, non-blocking review. May be wrong. Models: find mistral-small-4-119b-2603, gpt-oss-120b → verify gpt-5.4-mini → final gpt-5.5.
Re-checked at 2541249: nothing new.
|
…e them Signed-off-by: Leonardo Carreras <leonardo.carreras@eonerc.rwth-aachen.de>
2f5c66c to
2541249
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/pybind-enum-arg-repr #654 +/- ##
===========================================================
Coverage ? 64.91%
===========================================================
Files ? 533
Lines ? 37509
Branches ? 19937
===========================================================
Hits ? 24350
Misses ? 13158
Partials ? 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|



pybind11 resolves a parameter's type when
def()runs, so a class registered later in the module renders as its raw C++ name, for exampleCPS::IdentifiedObject,CPS::SystemTopologyorCPS::SimNode<double>. pybind11-stubgen degrades those to..., so the shipped stubs typedset_system,add_event,connectandconnect_componentas bare Any.Declares every
py::class_handle up front, populates the submodules, then attaches the.def()s, and splitsaddSignalComponentBases()out ofaddSignalComponents()to break the signal to base to dp dependency cycle. Together with the pull request below this takes pybind11-stubgen from 129 errors to 0, verified by rebuilding the module both ways on the same toolchain.