-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BMI docs page #1521
BMI docs page #1521
Conversation
docs/contribute/bmi.md
Outdated
|
||
- `user_demand.demand` yields the only 2D array, the other arrays are 1D. This array is indexed as `(node_idx, priority_idx)` | ||
- The index of e.g. basins and user demand nodes needs to be inferred from the Ribasim input. The same holds for `priority_idx`, which is global over all subnetworks | ||
- The `*_integrated` values are integrated from the start of the simulation onward. It might be beneficial to reset this to $0$ at certain times for long simulations to avoid loss of accuracy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend to add a note indicating that the flows are averaged over the timestep (if I am correct from t-1 tot t)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is different between what is exposed via the BMI and what is in the output. The output gives averaged values, the BMI exposes an integrated value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way the coupler has the freedom to decide over which period to average, and Ribasim doesn't have to worry about whether the coupler accesses the data before or after averaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you already indicate that `*_integrated' is the accumulation.
but your description of the other variables can be more precise, by indicating it is instantaneous, averaged or whatever it is. It now is still a bit open-ended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, that can be its own column. The differences between the BMI exposed data and the output are also worth mentioning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very complete, I have some minor text replacement suggestions:
Julia kernel is wrapped -> Ribasim provides, supports
listed function all correspond with same name functions in xmiwrapper but I am missing initialize_mpi to set up parallel from the coupler,
which is supported by XmiWrapper
"Set [...] timestep" -> "Perform/Advance [...] timestep"
"Set Ribasim internal timesteps until the specified time" -> "Advance/Forward etc. Ribasim exactly upto a demanded time"
Remove "Depending ... time specified", it is either ribasim internal stuff or duplicate (already mentioned and explained update_untill)
Mention that most of these functions correspond to methods in the XMIWrapper, but the get routines are usually not called from outside (coupler). X
The get_value_ptr is usually required by the XmiWrapper on the python side when obtaining a variables pointer, but usually not called outside the wrapper,
because the wrapper exposes the variables.
"internal array data" -> "internal array" or "internal memory"or just "variables are exposed"
Not only the data is exposed, but the memory. Important distinction when we want to write into Ribasim's memory
demand per node ID per priority -> demand per user and priority
time integrated intake flow per user -> time integrated realisation per user
"user_demand.demand yields the only 2D array, the other arrays are 1D. This array is indexed as (node_idx, priority_idx)" ->
should be clear from the indices in the last column of the table
Alternative for infiltration and drainage in the variables table to emphasize the distinction between instant and integrated volumes:
basin.infiltration = infiltration flux per basin
basin.drainage = drainage flux per basin
basin.infiltration_integrated = cumulative infiltration volume
basin.drainage_integrated = cumulative drainage volume
Same holds for user_demand.demand and user_demand.realized. The former is a flux (always implies m3/s), the latter is a cumulative volume.
I don't think an extra column is needed, the unit says it all
Maybe usefull: which other variables are also writable and can be used to set values. The bmi-user likes to know what happens if these variables can be changed from the outside how that influences Ribasim.
So maybe the last sentence more in general: "the _integrated values only accumulate during the simulation and can be set to zero (or any value) whenever suitable"
The user can think himself of the reason to do so.
docs/contribute/bmi.qmd
Outdated
|
||
Additional notes: | ||
|
||
- `user_demand.demand` yields the only 2D array, the other arrays are 1D. This array is indexed as `(node_idx, priority_idx)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nb. if memory presented as a one-dimensional array, runs fastest over priorities, then over user nodes, (therefore in a column major view such as in Julia or Fortran presented as (node_idx, priority_idx), but in row major views such as in Python as (priority_idx, node_idx)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look fine by me, I have one comment.
docs/contribute/bmi.qmd
Outdated
`basin.infiltration_integrated` | cumulative infiltration per basin | Float64 | $m^3$ | integrated from start | yes | basin node ID | ||
`basin.drainage_integrated` | cumulative drainage per basin | Float64 | $m^3$ | integrated from start | yes | basin node ID | ||
`basin.subgrid_level` | subgrid level | Float64 | $m$ | instantaneous | no | subgrid ID | ||
`user_demand.demand` | demand per node ID per priority | Float64 | $m^3 s^{-1}$ | forward fill | yes | user_demand node ID, priority index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please indicate if array is row or column major sorted
Fixes #1328.