Skip to content

Configurable FlowBoundary interpolation#2285

Merged
SouthEndMusic merged 17 commits into
mainfrom
feat/configurable-interpolation
Jun 3, 2025
Merged

Configurable FlowBoundary interpolation#2285
SouthEndMusic merged 17 commits into
mainfrom
feat/configurable-interpolation

Conversation

@SouthEndMusic

@SouthEndMusic SouthEndMusic commented May 20, 2025

Copy link
Copy Markdown
Collaborator

Fixes #2281. Also fixes #2244.

Results with schematizations that look like this:

model = Model(
    starttime="2020-01-01",
    endtime="2020-01-09",
    crs="EPSG:28992",
    interpolation=Interpolation(flow_boundary="stepwise", stepwise_smoothing=7200),
)

fb = model.flow_boundary.add(
    Node(1, Point(0, 0), cyclic_time=True),
    [
        flow_boundary.Time(
            time=["2020-01-01", "2020-01-02", "2020-01-03", "2020-01-04"],
            flow_rate=[1e-3, 2.5e-3, 0.0, 1e-3],
        )
    ],
)

...

Old behavior:

plot_8

New behavior with a smoothing period of 2 hours on either side of each data point:

plot_7

New behavior without smoothing:

plot_6

@SouthEndMusic SouthEndMusic changed the title POC Configurable FlowBoundary interpolation May 20, 2025
@SouthEndMusic SouthEndMusic marked this pull request as draft May 20, 2025 18:13
@SouthEndMusic SouthEndMusic marked this pull request as ready for review May 21, 2025 14:16
@SouthEndMusic SouthEndMusic requested review from verheem and visr May 21, 2025 14:20
@visr

visr commented May 22, 2025

Copy link
Copy Markdown
Member

This looks nice! Do you have any benchmark numbers that compare these three options, linear, stepwise hard, stepwise smooth?

@SouthEndMusic

Copy link
Copy Markdown
Collaborator Author

While making the benchmarks I came across a DataInterpolations bug:

SciML/DataInterpolations.jl#432

We need this to add the right tstops to our integrator if we use SmoothedConstantInterpolation

@SouthEndMusic

Copy link
Copy Markdown
Collaborator Author

Benchmarks:

  • Stepwise interpolation without smoothing
BenchmarkTools.Trial: 184 samples with 1 evaluation per sample.
 Range (min … max):  5.859 ms … 33.363 ms  ┊ GC (min … max): 0.00% … 60.40%
 Time  (median):     6.862 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   7.431 ms ±  2.462 ms  ┊ GC (mean ± σ):  4.38% ±  9.25%

     ▄█▇▇▄
  ▄▇▅██████▅▇▇▆▅▆▃▁▄▃▃▃▁▃▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃▃▁▁▁▃▁▁▃▁▁▁▁▁▃▁▃▁▁▃ ▃
  5.86 ms        Histogram: frequency by time        14.6 ms <
  • Stepwise interpolation with smoothing of 7200 s
BenchmarkTools.Trial: 157 samples with 1 evaluation per sample.
 Range (min … max):   9.348 ms … 31.975 ms  ┊ GC (min … max): 0.00% … 53.83%
 Time  (median):     10.478 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   11.576 ms ±  3.008 ms  ┊ GC (mean ± σ):  6.19% ± 10.89%

     ▃▆█▃  ▃
  ▇▅▇█████▃█▅▅▃▄▄▃▄▃▃▃▁▃▁▁▁▁▁▁▁▁▁▁▃▁▃▁▃▁▃▁▃▁▁▁▁▁▃▁▃▃▃▁▄▁▃▁▁▃▃ ▃
  9.35 ms         Histogram: frequency by time        19.6 ms <
  • Linear interpolation:
BenchmarkTools.Trial: 170 samples with 1 evaluation per sample.
 Range (min … max):  6.429 ms … 31.650 ms  ┊ GC (min … max): 0.00% … 61.35%
 Time  (median):     7.653 ms              ┊ GC (median):    0.00%
 Time  (mean ± σ):   8.280 ms ±  2.587 ms  ┊ GC (mean ± σ):  4.32% ±  9.10%

    ▁ █▄▁▁▂
  ▄█████████▅▆▅▄▃▄▄▃▅▃▁▁▁▃▁▁▃▁▃▁▁▁▁▁▁▃▁▁▁▃▁▁▁▁▁▁▁▁▃▃▁▁▁▃▁▁▃▃ ▃
  6.43 ms        Histogram: frequency by time        16.4 ms <

 Memory estimate: 2.59 MiB, allocs estimate: 19759.

Notes:

  • The DataInterpolations bug mentioned above is now solved on main
  • Stepwise interpolation with smoothing leads to more tstops, so that probably has a significant impact here. It would be nice if we can see a significant performance boost w.r.t. not smoothing in more complex models
  • Running these benchmarks gave me a nice demonstration of our latency problems, because the eltype of flow_boundary.flow_rate now depends on the new config option and changing the interpolation type leads to significant recompilation. @evetion do you think it would be better to change the eltype of flow_boundary.flow_rate to a union of the supported types, given that that union will remain small? That would mean some refactoring for the parameter parsing code

Comment thread core/test/docs.toml Outdated
Comment thread python/ribasim_testmodels/ribasim_testmodels/basic.py Outdated
Comment thread docs/reference/usage.qmd Outdated
## Interpolation settings
There are the following interpolation settings:
- `flow_boundary`: The interpolation type of flow boundary timeseries. This is `linear` by default, but can also be set to `stepwise`.
- `stepwise_smoothing`: When an interpolation type is set to `stepwise`, this parameter determines an interval in time on either side of each data point which is used to smooth the transition between data points. See also the [documentation](https://docs.sciml.ai/DataInterpolations/dev/methods/#Smoothed-Constant-Interpolation) for this interpolation type.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we keep the configuration as small/simple as possible, but if we need different interpolation types, are they global (all nodes), configurable per nodetype (all flowboundaries are stepwise) or configurable per node (😱)?

If per node(type), then we should have a config group with specific settings per nodetype. Otherwise we have a breaking change once we introduce this. Or, we determine stepwise_smoothing automatically and don't leave it up to the user.

@visr

visr commented Jun 2, 2025

Copy link
Copy Markdown
Member

As discussed today, we'll go with block interpolation with 0 smoothing as a default. This PR needs to be updated to reflect that.

@SouthEndMusic SouthEndMusic requested a review from visr June 2, 2025 11:51
@SouthEndMusic SouthEndMusic merged commit 204b016 into main Jun 3, 2025
19 checks passed
@SouthEndMusic SouthEndMusic deleted the feat/configurable-interpolation branch June 3, 2025 11:41
@visr visr mentioned this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linear interpolation of daily inflow data causes peak smoothing Replace ConstantInterpolation by SmoothedConstantInterpolation

3 participants