Runtime bug with DeficitParameter

[s-simoncelli]

If one defines a parameter as:

{
    "meta": {
        "name": "Total deficit"
    },
    "type": "Aggregated",
    "agg_func": {
        "type": "Sum"
    },
    "metrics": [
        {
            "type": "Node",
            "name": "demand1",
            "attribute": "Deficit"
        },
        {
            "type": "Node",
            "name": "demand2",
            "attribute": "Deficit"
        }
    ]
}

The model panics at the first time step with:

Error stepping through model: Error stepping through network at timestep Timestep {
    date: 2100-01-01T00:00:00,
    index: 0,
    duration: PywrDuration(
        TimeDelta {
            secs: 604800,
            nanos: 0,
        },
    ),
}: Error calculating value for parameter `Total deficit`: F64 metric error: State error: General parameter index not found: 2

I originally implemented the deficit metric in the output node via the DeficitParameter, but apparently it is not properly registered in the state. The new implementation moves the logic from the parameter to a new MetricF64 metric and remove DeficitParameter considering that the parameter was not public in the schema and this approach is consistent with how other metrics are defined.

[jetuk]

I've had a quick look at this. I think there's a wider issue with the changes brought in #599. I.e. what happens if you use an aggregated parameter on parameters that only return a value in "after". The DeficitParameter returns None in before which is getting turned into an index not found error.

I think the actual issue is that the None is getting flattened (

pywr-next/pywr-core/src/state/mod.rs

Line 261 in bdb1fb5

self.values.get(idx).copied().flatten()

) with the option returned from Vec::get. Therefore, the code path can no longer distinguish between a missing state entry (which should not happen) and an explicit None returned from before. That probably needs fixing such that the former is an error.

Then the logic in any parameters consuming others needs updating somehow. The metric API requires a Result<f64, ...>, so any missing parameter value would need making a separate error variant. For AggregatedParameter, if it tries to aggregate in "before" and only finds missing parameters then it itself returns None. If some are missing and others are not, that seems like a model definition error.

The reason for not making the deficit a Metric variant directly is that it will cause it to be recalculated at annoying times. For examples, at the beginning of the timestep you might get yesterday's flow compared with today's demand.

[jetuk]

I have addressed this problem in #745. Would you mind having a look at that and see if it works for your case?