Add dynamic Preissmann slot support by wiesnerfriedman · Pull Request #55 · HydroCouple/openswmm.engine

wiesnerfriedman · 2026-04-16T01:24:06Z

No description provided.

Copilot

Pull request overview

Adds a new GoogleTest suite intended to validate the Dynamic Preissmann Slot (DPS) behavior in the dynamic wave solver, and wires it into the unit test build.

Changes:

Introduces a large DPS-focused unit test file covering parameter defaults, decay behavior, surcharge onset, geometry, and edge cases.
Registers a new test_engine_dps test target in the unit-test CMakeLists.
Also adds test_engine_site_drainage to the unit test target list.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
tests/unit/engine/test_dynamic_preissmann_slot.cpp	Adds extensive DPS tests and helper scaffolding for a minimal simulation context.
tests/unit/engine/CMakeLists.txt	Adds new gtest executables for the DPS tests and site drainage model test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T01:02:21Z

+TEST(DPSConstants, DefaultTargetCelerity) {
+    EXPECT_DOUBLE_EQ(DPS_DEFAULT_TARGET_CELERITY, 100.0);
+}
+
+TEST(DPSConstants, DefaultShockParam) {
+    EXPECT_DOUBLE_EQ(DPS_DEFAULT_SHOCK_PARAM, 2.0);
+}
+
+TEST(DPSConstants, DefaultDecayTime) {
+    EXPECT_DOUBLE_EQ(DPS_DEFAULT_DECAY_TIME, 10.0);
+}
+
+TEST(DPSConstants, CrownCutoffMatchesSlot) {
+    EXPECT_DOUBLE_EQ(DPS_CROWN_CUTOFF, SLOT_CROWN_CUTOFF);
+}
+
+TEST(DPSConstants, DynamicSlotEnumValue) {
+    EXPECT_EQ(static_cast<int>(SurchargeMethod::DYNAMIC_SLOT), 2);
+}
+
+TEST(DPSSolverDefaults, DefaultDPSParameters) {
+    DWSolver solver;
+    EXPECT_DOUBLE_EQ(solver.dps_target_celerity, DPS_DEFAULT_TARGET_CELERITY);
+    EXPECT_DOUBLE_EQ(solver.dps_shock_param, DPS_DEFAULT_SHOCK_PARAM);
+    EXPECT_DOUBLE_EQ(solver.dps_decay_time, DPS_DEFAULT_DECAY_TIME);
+}


This test references DPS_* constants (e.g., DPS_DEFAULT_TARGET_CELERITY, DPS_DEFAULT_SHOCK_PARAM, DPS_DEFAULT_DECAY_TIME, DPS_CROWN_CUTOFF) that don't exist in the current codebase (DPS parameters live in SimulationOptions and internal DWSolver::dps_config_). Update the tests to read defaults from SimulationOptions (ctx.options.*) or validate behavior through the public solver API instead of undeclared constants.

Copilot · 2026-04-20T01:02:21Z

+        solver.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;
+        solver.init(2, 1, groups);


DWSolver::init currently requires a SimulationContext parameter (init(n_nodes, n_links, groups, ctx)), but these tests call init(…, groups) without passing ctx. As written this won't compile and also bypasses DPS option initialization (ctx.options.dps_). Pass the built SimulationContext into init and set ctx.options.surcharge_method/dps_ appropriately for each test case.

Suggested change

solver.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;

solver.init(2, 1, groups);

ctx.options.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;

ctx.options.dps_target_celerity = DPS_DEFAULT_TARGET_CELERITY;

ctx.options.dps_shock_param = DPS_DEFAULT_SHOCK_PARAM;

ctx.options.dps_decay_time = DPS_DEFAULT_DECAY_TIME;

solver.init(2, 1, groups, ctx);

Copilot · 2026-04-20T01:02:22Z

+
+    void setSurchargedState(double p0, double surcharge_time) {
+        solver.dps_preissmann_[0] = p0;
+        solver.dps_surcharge_t_[0] = surcharge_time;
+    }
+};
+
+TEST_F(DPSDecayTest, AtTimeZeroReturnsP0) {
+    double p0 = 5.0;
+    setSurchargedState(p0, 0.0);
+
+    // At t=0: P = 1 - (1 - P0) * exp(0) = 1 - (1 - P0) = P0
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_NEAR(P, p0, 1e-10);
+}
+
+TEST_F(DPSDecayTest, DecaysToward1) {
+    double p0 = 10.0;
+    setSurchargedState(p0, 0.0);
+    double P_at_0 = solver.computePreissmannNumber(0, 0.0);
+
+    setSurchargedState(p0, 50.0);  // 5 time constants
+    double P_at_50 = solver.computePreissmannNumber(0, 0.0);
+
+    EXPECT_GT(P_at_0, P_at_50);
+    EXPECT_NEAR(P_at_50, 1.0, 0.1);  // Should be very close to 1 after 5τ


These tests access DWSolver internals (e.g., updateDPSState, getSlotWidth/getCrownCutoff, and dps_* state vectors) that are private in DynamicWave.hpp and/or no longer exist as public members (DPS state is stored in private dps_state_). Either refactor the tests to validate DPS via public solver behavior (execute/geometry outputs), or add an explicit test-only access mechanism (e.g., friend test, public debug getters behind a compile flag).

Suggested change

void setSurchargedState(double p0, double surcharge_time) {

solver.dps_preissmann_[0] = p0;

solver.dps_surcharge_t_[0] = surcharge_time;

}

};

TEST_F(DPSDecayTest, AtTimeZeroReturnsP0) {

double p0 = 5.0;

setSurchargedState(p0, 0.0);

// At t=0: P = 1 - (1 - P0) * exp(0) = 1 - (1 - P0) = P0

double P = solver.computePreissmannNumber(0, 0.0);

EXPECT_NEAR(P, p0, 1e-10);

}

TEST_F(DPSDecayTest, DecaysToward1) {

double p0 = 10.0;

setSurchargedState(p0, 0.0);

double P_at_0 = solver.computePreissmannNumber(0, 0.0);

setSurchargedState(p0, 50.0); // 5 time constants

double P_at_50 = solver.computePreissmannNumber(0, 0.0);

EXPECT_GT(P_at_0, P_at_50);

EXPECT_NEAR(P_at_50, 1.0, 0.1); // Should be very close to 1 after 5τ

};

TEST_F(DPSDecayTest, FreshlyInitializedLinkHasUnityPreissmannNumber) {

// Without any surcharge history introduced through the public API,

// the solver should report the neutral/default DPS state.

double P = solver.computePreissmannNumber(0, 0.0);

EXPECT_NEAR(P, 1.0, 1e-10);

}

TEST_F(DPSDecayTest, UnsurchargedLinkRemainsAtUnityOverTime) {

// A link with no surcharge state should remain at the baseline

// Preissmann number regardless of elapsed time.

double P_at_0 = solver.computePreissmannNumber(0, 0.0);

double P_at_50 = solver.computePreissmannNumber(0, 50.0);

EXPECT_NEAR(P_at_0, 1.0, 1e-10);

EXPECT_NEAR(P_at_50, 1.0, 1e-10);

Copilot · 2026-04-20T01:02:22Z

+TEST(DPSSlotBehavior, SlotWidthUsesSjobergFormula) {
+    // For DYNAMIC_SLOT, at depth = y_full * 0.99 (above SLOT_CROWN_CUTOFF),
+    // the Sjoberg formula should give a positive width.
+    DWSolver solver;
+    solver.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;
+
+    double y_full = 3.0;
+    double w_max = 3.0;
+    double y = y_full * 0.99;
+    double w = solver.getSlotWidth(y, y_full, w_max, XsectShape::CIRCULAR);
+
+    EXPECT_GT(w, 0.0);
+
+    // Should match what SLOT method gives
+    DWSolver solver_slot;
+    solver_slot.surcharge_method = SurchargeMethod::SLOT;
+    double w_slot = solver_slot.getSlotWidth(y, y_full, w_max, XsectShape::CIRCULAR);
+
+    EXPECT_DOUBLE_EQ(w, w_slot);
+}


The slot-width behavior asserted here doesn't match the current implementation: DynamicWave.cpp::getSlotWidth() returns a non-zero width only when surcharge_method == SLOT (DYNAMIC_SLOT returns 0). If DYNAMIC_SLOT should share Sjoberg slot-width behavior, the implementation needs to be updated; otherwise these tests should be removed/rewritten to check the DPS geometry override path (applyDPSGeometry) instead of getSlotWidth().

Copilot · 2026-04-20T01:02:22Z

+TEST_F(DPSDecayTest, AtTimeZeroReturnsP0) {
+    double p0 = 5.0;
+    setSurchargedState(p0, 0.0);
+
+    // At t=0: P = 1 - (1 - P0) * exp(0) = 1 - (1 - P0) = P0
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_NEAR(P, p0, 1e-10);
+}
+
+TEST_F(DPSDecayTest, DecaysToward1) {
+    double p0 = 10.0;
+    setSurchargedState(p0, 0.0);
+    double P_at_0 = solver.computePreissmannNumber(0, 0.0);
+
+    setSurchargedState(p0, 50.0);  // 5 time constants
+    double P_at_50 = solver.computePreissmannNumber(0, 0.0);
+
+    EXPECT_GT(P_at_0, P_at_50);
+    EXPECT_NEAR(P_at_50, 1.0, 0.1);  // Should be very close to 1 after 5τ
+}
+
+TEST_F(DPSDecayTest, ExponentialDecayVerification) {
+    double p0 = 8.0;
+    double r = solver.dps_decay_time;  // 10 s
+    double t = 5.0;  // half a time constant
+
+    setSurchargedState(p0, t);
+    double P = solver.computePreissmannNumber(0, 0.0);
+
+    double expected = 1.0 - (1.0 - p0) * std::exp(-t / r);
+    EXPECT_NEAR(P, expected, 1e-10);
+}
+
+TEST_F(DPSDecayTest, AtInfiniteTimeConvergesTo1) {
+    double p0 = 20.0;
+    setSurchargedState(p0, 1e6);  // Very long time
+
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_NEAR(P, 1.0, 1e-6);
+}
+
+TEST_F(DPSDecayTest, ZeroDecayTimeReturns1) {
+    solver.dps_decay_time = 0.0;
+    setSurchargedState(5.0, 1.0);
+
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_DOUBLE_EQ(P, 1.0);
+}
+
+TEST_F(DPSDecayTest, NotSurchargedReturnsCurrent) {
+    solver.dps_preissmann_[0] = 7.5;
+    solver.dps_surcharge_t_[0] = -1.0;  // Not surcharged
+
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_DOUBLE_EQ(P, 7.5);
+}
+
+TEST_F(DPSDecayTest, NeverBelowOne) {
+    // Even with P0 < 1 (forced), result should be >= 1
+    setSurchargedState(0.5, 2.0);  // P0 < 1 (shouldn't happen normally)
+    double P = solver.computePreissmannNumber(0, 0.0);
+    EXPECT_GE(P, 1.0);
+}


The decay model asserted in these tests (Eq. 22 as exp(-t/r) and using dps_surcharge_t_ directly) does not match the current DPS implementation: updateDPSState() computes P_hat using exp(-10*(t - t_s)/r) and tracks surcharge onset via dps_state_.t_s with internal sim_time_. The tests should be aligned with the actual formulation used in DynamicWave.cpp (or the implementation updated if the tests represent the intended model).

Copilot · 2026-04-20T01:02:22Z

+add_gtest_unit(test_engine_dps              test_dynamic_preissmann_slot.cpp)
+add_gtest_unit(test_engine_site_drainage    test_site_drainage_model.cpp)


This test suite enables test_site_drainage_model.cpp as a unit test target. That file is an integration-style test that runs a full model from an .inp in the data directory (and can execute ~20k+ steps). Consider gating it behind an option (e.g., OPENSWMM_WITH_INTEGRATION_TESTS) and/or labeling it as "integration" instead of "unit" to avoid slowing or destabilizing the unit-test CI signal.

Add dynamic Preissmann slot support

62741b9

cbuahin requested a review from Copilot April 20, 2026 00:58

Copilot started reviewing on behalf of cbuahin April 20, 2026 00:58 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

cbuahin self-requested a review April 20, 2026 02:05

Fix DPS unit tests and gate integration build

bec9d4c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic Preissmann slot support#55

Add dynamic Preissmann slot support#55
wiesnerfriedman wants to merge 2 commits intoHydroCouple:swmm6_relfrom
wiesnerfriedman:pr/dps-testing

wiesnerfriedman commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		solver.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;
		solver.init(2, 1, groups);

-        solver.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;
-        solver.init(2, 1, groups);
+        ctx.options.surcharge_method = SurchargeMethod::DYNAMIC_SLOT;
+        ctx.options.dps_target_celerity = DPS_DEFAULT_TARGET_CELERITY;
+        ctx.options.dps_shock_param = DPS_DEFAULT_SHOCK_PARAM;
+        ctx.options.dps_decay_time = DPS_DEFAULT_DECAY_TIME;
+        solver.init(2, 1, groups, ctx);

		add_gtest_unit(test_engine_dps test_dynamic_preissmann_slot.cpp)
		add_gtest_unit(test_engine_site_drainage test_site_drainage_model.cpp)

Conversation

wiesnerfriedman commented Apr 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants