Skip to content
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

Merged
merged 6 commits into from
Jun 12, 2024
Merged

BMI docs page #1521

merged 6 commits into from
Jun 12, 2024

Conversation

SouthEndMusic
Copy link
Collaborator

Fixes #1328.


- `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.
Copy link
Contributor

@gijsber gijsber Jun 5, 2024

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)

Copy link
Collaborator Author

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

Copy link
Collaborator Author

@SouthEndMusic SouthEndMusic Jun 5, 2024

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

Copy link
Contributor

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

Copy link
Collaborator Author

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

Copy link
Collaborator

@rleander rleander left a 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.


Additional notes:

- `user_demand.demand` yields the only 2D array, the other arrays are 1D. This array is indexed as `(node_idx, priority_idx)`
Copy link
Collaborator

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)

Copy link

@HendrikKok HendrikKok left a 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.

`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

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

@SouthEndMusic SouthEndMusic merged commit a596627 into main Jun 12, 2024
24 of 25 checks passed
@SouthEndMusic SouthEndMusic deleted the bmi_docs_page branch June 12, 2024 10:09
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.

Add BMI page to docs
4 participants