feat: Allow specifying calculation phase in some parameters. by jetuk · Pull Request #745 · pywr/pywr-next

jetuk · 2026-08-10T13:18:50Z

This folows on from #726 to do the following:

pywr-core: Internally allocate space only for before and after parameters that are registered to supply values in those phases. Two new index types are added for this.
pywr-core: Validate consumer phasing with parameter return values during network build. This should reject invalid combinations at build time and avoid spurious missing values at runtime.
pywr-schema: Allow specifying the phase for Aggregated and AggregatedIndex parameters in the schema.
pywr-schema: Correctly construct parameter references for appropriate phase when building metric sets from all parameters.
pywr-schema: Add tests models for the issue identified in Runtime bug with DeficitParameter #725 (i.e. calculating and saving total deficit).

What's left to do is implement alternative phase (i.e. after) for some of the other parameter types.

This folows on from #726 to do the following: - pywr-core: Internally allocate space only for before and after parameters that are registered to supply values in those phases. Two new index types are added for this. - pywr-core: Validate consumer phasing with parameter return values during network build. This should reject invalid combinations at build time and avoid spurious missing values at runtime. - pywr-schema: Allow specifying the phase for Aggregated and AggregatedIndex parameters in the schema. - pywr-schema: Correctly construct parameter references for appropriate phase when building metric sets from all parameters. - pywr-schema: Add tests models for the issue identified in #725 (i.e. calculating and saving total deficit). What's left to do is implement alternative phase (i.e. after) for some of the other parameter types.

Batch21

I've gone through this in some detail and think the approach is good. A few queries/suggestions below.

Batch21 · 2026-08-15T09:59:58Z

+        let metrics = resolve_metric_u64_vec!(self, &self.metrics, resolution_maps, self.phase, "metrics");

        let meta = self.meta;
        let agg_func = self.agg_func;


does the builder need to match on phase when processing the metrics like in AggregatedParameterBuilder?

Batch21 · 2026-08-15T10:36:48Z

-    }
-
-    #[test]
-    fn simple_before_parameter_can_depend_on_simple_before_value() {


do these removed tests need replacing or updating?

I have added more unit tests.

Batch21 · 2026-08-15T10:49:42Z

+fn resolve_parameter_index_f64_to_metric_f64(
+    name: &ParameterName,


Should we have some unit tests that check that the correct errors are produced for different combinations of return values and phases.

Also, would it be possible to combined some of the logic from the resolve_parameter_index_* functions into a share function?

Batch21 · 2026-08-15T11:03:32Z

+            Self::Polynomial1D(_) => ParameterPhase::Before,
+            Self::Threshold(_) => ParameterPhase::Before,
+            Self::TablesArray(_) => ParameterPhase::Before,
+            Self::Python(_) => ParameterPhase::Before,


The Python parameter could be After or Both which could cause an error when this is used by by metrics sets. Do these phases need to be determined dynamically from the registered parameters instead?

Currently the parameter (in core) is hardcoded to be "before". I'll fix this along with other parameters in a follow-up.

Batch21 · 2026-08-15T11:06:57Z

+    match idx {
+        // Constant and simple can always be resolved to a metric, regardless of the consumer phase
+        // as long as the parameter return value is "before".
+        ParameterIndex::Const(idx) => match parameter_return_value {


The other resolve_parameter_index_* function do not match on parameter_return_value from constant paramters like this one does.

Batch21 · 2026-08-15T11:39:42Z

+        // Determine which calculation phase(s) the parameter will be used in
+        // based on whether the target and actual flow are provided.
+        // If neither is provided, return an error.
+        let phase = match (self.target.is_some(), self.actual_flow.is_some()) {


won't the phase here vary across the metrics? some will be access in before and some in after?

Isn't that what the match statement is doing? Working out which phases this parameter will be in?

Batch21 · 2026-08-15T11:49:32Z

+                    // initial value is acceptable in the "before" phase, so we can resolve it to a metric provided the
+                    // parameter index contains an "after" index.
+                    match idx.after {
+                        Some(after_idx) => Ok(MetricF64::ParameterAfterMulti {


This could result in a runtime error as it will try and look up a value in an empty hashmap?

Yes, that's true. I think a follow-up idea would allow parameters to provide initial values.

Batch21 · 2026-08-15T12:13:23Z

 impl AsymmetricSwitchIndexParameterBuilder {
-    pub fn new(name: ParameterName, on_parameter: UnresolvedMetricU64, off_parameter: UnresolvedMetricU64) -> Self {
+    /// Create a new builder for [`AsymmetricSwitchIndexParameter`] that is evaluated in the "before" phase.
+    pub fn before(name: ParameterName, on_parameter: UnresolvedMetricU64, off_parameter: UnresolvedMetricU64) -> Self {


Some parameters, like this one, define a new before constructor method while others (like the max parameter) hardcode a before phase variable in their existing build methods. Is there a reasons for this? Should it be made consistent?

I think this was just a bit of WIP. I was going to do this as the main framework, and then go back through and implement it for other parameters. So, I renamed this one but it doesn't support after at the moment.

Batch21 · 2026-08-15T12:14:29Z

@@ -0,0 +1,133 @@
+{


This model isn't tested anywhere?

Batch21 · 2026-08-15T12:16:09Z


 impl AggregatedParameterBuilder {
-    pub fn new(name: ParameterName, agg_func: AggFuncF64) -> Self {
+    /// Create a new builder for [`AggregatedParameter`] that is evaluated in the "after" phase.


…4 or multi parameters.

A new error type to fail a build if a metric would not be used.

jetuk added 2 commits August 10, 2026 14:12

Fix feature gating imports.

06de4c7

jetuk mentioned this pull request Aug 12, 2026

Runtime bug with DeficitParameter #725

Open

Batch21 requested changes Aug 15, 2026

View reviewed changes

jetuk added 10 commits August 16, 2026 17:37

Add parameter build cycle, dependency and error reporting tests.

89c04ac

Add tests on parameter collection size and name indices.

7bab67d

Add additional unit tests for parameter collection.

ba03623

Add some network level integration tests.

1fd9631

refactor: Move test parameters to their own module.

7be1904

fix: Respect the phase when building AggregatedIndexParameter.

e7eb99a

docs: Fix AggregatedParameterBuilder::before docstring.

d3b8635

fix: Check phase we create simple or const metrics from unresolved u6…

a803e83

…4 or multi parameters.

Fix issues with schema level tests.

f0d3d63

fix: Handle unused parameters in HydroPowerTargetParameterBuilder.

a4a0815

A new error type to fail a build if a metric would not be used.

		fn resolve_parameter_index_f64_to_metric_f64(
		name: &ParameterName,

Uh oh!

Conversation

jetuk commented Aug 10, 2026

Uh oh!

Batch21 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!

jetuk Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

2 participants

jetuk Aug 17, 2026 •

edited

Loading