Add results to xugrid #1369
Add results to xugrid #1369
Conversation
python/ribasim/ribasim/model.py
Outdated
| @@ -249,7 +249,7 @@ def validate_model(self): | |||
| @classmethod | |||
| def read(cls, filepath: str | PathLike[str]) -> "Model": | |||
| """Read model from TOML file.""" | |||
| return cls(filepath=filepath) # type: ignore | |||
| return cls(filepath=Path(filepath)) # type: ignore | |||
There was a problem hiding this comment.
pydantic should take care of this.
Did you have troubles without this change?
There was a problem hiding this comment.
Oh right, I just couldn't find where it was, but it is in FileModel. Reverted this line.
| @@ -264,10 +264,10 @@ def write(self, filepath: str | PathLike[str]) -> Path: | |||
| # TODO | |||
| # self.validate_model() | |||
| filepath = Path(filepath) | |||
| self.filepath = filepath | |||
There was a problem hiding this comment.
Looking at this, it feels a bit weird that we store self.filepath, but then not use it here.
Should the filepath be maybe optional for write?
There was a problem hiding this comment.
We could make filepath optional for write, though then we are encouraging overwriting models, and it would perhaps not be obvious to code readers where their model ended up.
This feels ok to me since once an in-memory model is written to disk, it has a filepath. Otherwise that would only be set when reading a model with Model.read("path/to/ribasim.toml").
| def _add_results(self, uds): | ||
| toml_path = self.filepath | ||
| if toml_path is None: | ||
| raise FileNotFoundError("Model must be written to disk to add results.") |
There was a problem hiding this comment.
Does FileNotFoundError and the error message make sense here?
The error happens when self.filepath is not set, so you could just say that
There was a problem hiding this comment.
Isn't self.filepath more of an implementation detail? Users would maybe try model.filepath = "path/to/ribasim.toml", and then it would probably fail at the next check when the results don't exist.
|
I think this comes down to the choice that QGIS has made when it comes to visualizing data located on vertices. It's not obvious what the right choice is, in xugrid, I've chosen points: uds["storage"].max("time").ugrid.plot()
You can get something weird in QGIS if you set a really thick line thickness...
Polygon support is tricky. UGRID won't support it, unless you convert the polygons to a mesh topology. You can do this cheaply: https://github.com/skogler/mapbox_earcut_python Thinking along these lines: what's also possible is to generate a 2D mesh with one face per node. So you generate e.g. a hexagon for each basin, and assign the vertex data to those faces. If you want to be cute, you can even reflect the shape of the QGIS plugin nodes in the output 2D topology... Having actual polygons requires geoarrow / geopackage etc. And unless they changed how QGIS works, it's not a great match for the temporal controller, because it ends up filtering all rows instead of using an Index like it does for MDAL datasets and so it has unimpressive performance (especially with microsecond output...). |
Ideally both would be options in QGIS. At least we can use xugrid for this analysis :). I feels like the way we store the data here is correct, it is just a pity that it is not yet possible to visualize this nicely in QGIS. But I feel like the 2D mesh with one face per node idea is not really an option, since we don't know how large to make the faces. You want this to be a fixed size on screen such that it is properly visible on different zoom levels. For polygons I feel like it's worth double checking the performance for a real model now that we only |
You could let the size depend somewhat on the original size of the basin polygon. Maybe try an inscribed circle to find a radius, then use the radius to define a hexagon? :P |
|
Hehe too clever, may as well require 'Basin / area' polygons then. But thanks for the suggestions, I will try some. I think this can be merged like this. |
Small bug I didn't notice in #1369 because the basic example has the same number of nodes and edges.
Fixes #1833 HWS output looks like this before, and no output after. The `invalid_unstable` test model still logs though, as captured by tests. ``` ┌ Info: Convergence bottlenecks in descending order of severity: │ Pump #2019 = NaN │ Pump #2130 = NaN │ Pump #2315 = NaN │ Pump #2637 = NaN │ Pump #3995 = NaN │ Pump #4536 = NaN │ Pump #6128 = NaN │ Pump #6866 = NaN │ Pump #9633 = NaN │ Pump #10704 = NaN │ Pump #10710 = NaN │ Pump #10711 = NaN │ Pump #10712 = NaN │ Pump #10713 = NaN │ Pump #10714 = NaN │ Pump #10715 = NaN │ Pump #10716 = NaN │ Pump #10717 = NaN │ Outlet #6692 = NaN │ Outlet #7679 = NaN │ Outlet #10702 = NaN │ Outlet #10703 = NaN │ UserDemand #1000015 = NaN │ UserDemand #1000016 = NaN │ UserDemand #1000017 = NaN │ UserDemand #1000018 = NaN │ UserDemand #1000019 = NaN │ UserDemand #1000020 = NaN │ UserDemand #1000021 = NaN │ UserDemand #1000022 = NaN │ UserDemand #1000038 = NaN │ UserDemand #1000040 = NaN │ UserDemand #1000042 = NaN │ Basin #529 = NaN │ Basin #670 = NaN │ Basin #905 = NaN │ Basin #1041 = NaN │ Basin #1340 = NaN │ Basin #1369 = NaN │ Basin #1514 = NaN │ Basin #1676 = NaN │ Basin #1709 = NaN │ Basin #1873 = NaN │ Basin #2028 = NaN │ Basin #2232 = NaN │ Basin #2255 = NaN │ Basin #2323 = NaN │ Basin #2336 = NaN │ Basin #2381 = NaN │ Basin #2468 = NaN │ Basin #2496 = NaN │ Basin #2522 = NaN │ Basin #2590 = NaN │ Basin #2749 = NaN │ Basin #2968 = NaN │ Basin #3016 = NaN │ Basin #3090 = NaN │ Basin #3302 = NaN │ Basin #3364 = NaN │ Basin #3448 = NaN │ Basin #3543 = NaN │ Basin #3584 = NaN │ Basin #3721 = NaN │ Basin #3899 = NaN │ Basin #4024 = NaN │ Basin #4134 = NaN │ Basin #4184 = NaN │ Basin #4279 = NaN │ Basin #4339 = NaN │ Basin #4473 = NaN │ Basin #4570 = NaN │ Basin #4714 = NaN │ Basin #4796 = NaN │ Basin #4830 = NaN │ Basin #4838 = NaN │ Basin #5048 = NaN │ Basin #5097 = NaN │ Basin #5207 = NaN │ Basin #5322 = NaN │ Basin #5371 = NaN │ Basin #5381 = NaN │ Basin #5472 = NaN │ Basin #5498 = NaN │ Basin #5505 = NaN │ Basin #5530 = NaN │ Basin #5616 = NaN │ Basin #5909 = NaN │ Basin #5978 = NaN │ Basin #5999 = NaN │ Basin #6017 = NaN │ Basin #6077 = NaN │ Basin #6101 = NaN │ Basin #6164 = NaN │ Basin #6224 = NaN │ Basin #6273 = NaN │ Basin #6327 = NaN │ Basin #6364 = NaN │ Basin #6401 = NaN │ Basin #6419 = NaN │ Basin #6428 = NaN │ Basin #6437 = NaN │ Basin #6473 = NaN │ Basin #6503 = NaN │ Basin #6582 = NaN │ Basin #6757 = NaN │ Basin #6885 = NaN │ Basin #6926 = NaN │ Basin #6945 = NaN │ Basin #6972 = NaN │ Basin #7053 = NaN │ Basin #7087 = NaN │ Basin #7101 = NaN │ Basin #7117 = NaN │ Basin #7164 = NaN │ Basin #7214 = NaN │ Basin #7310 = NaN │ Basin #7385 = NaN │ Basin #7400 = NaN │ Basin #7417 = NaN │ Basin #7484 = NaN │ Basin #7597 = NaN │ Basin #7615 = NaN │ Basin #7668 = NaN │ Basin #7706 = NaN │ Basin #7843 = NaN │ Basin #7909 = NaN │ Basin #8014 = NaN │ Basin #8020 = NaN │ Basin #8049 = NaN │ Basin #8059 = NaN │ Basin #8071 = NaN │ Basin #8234 = NaN │ Basin #8257 = NaN │ Basin #8340 = NaN │ Basin #8364 = NaN │ Basin #8559 = NaN │ Basin #8685 = NaN │ Basin #8717 = NaN │ Basin #8757 = NaN │ Basin #8786 = NaN │ Basin #8817 = NaN │ Basin #8921 = NaN │ Basin #9033 = NaN │ Basin #9086 = NaN │ Basin #9123 = NaN │ Basin #9144 = NaN │ Basin #9185 = NaN │ Basin #9209 = NaN │ Basin #9358 = NaN │ Basin #9406 = NaN │ Basin #9430 = NaN │ Basin #9484 = NaN │ Basin #9586 = NaN │ Basin #9627 = NaN │ Basin #9830 = NaN │ Basin #9885 = NaN │ Basin #9899 = NaN │ Basin #9906 = NaN │ Basin #9954 = NaN │ Basin #10040 = NaN │ Basin #10107 = NaN │ Basin #10184 = NaN │ Basin #10202 = NaN │ Basin #10308 = NaN │ Basin #10321 = NaN │ Basin #10343 = NaN │ Basin #10583 = NaN │ Basin #10587 = NaN │ Basin #10634 = NaN └ Basin #10659 = NaN ```
Part of #969
We cannot fully test this since we don't yet support testing results from Python.
This results in the following netCDF: basic.zip
It has
flow_rateon the edges, and all variables from basin.arrow on the nodes.The edge visualization is fine, and it supports the QGIS temporal controller:
I'm not yet happy with the visualization on nodes. Maybe @Huite has ideas on how to improve this?
The problem is that the
udshas all nodes, but we only have Basin results on Basins, so the rest is NaN. If there is a NaN in between QGIS doesn't draw the line. If instead we putuds[var_name] = da.reindex_like(uds, fill_value=0.0)then it looks more like the flow_rate image, though that isn't what we want.Ideally we get some nice clear points. Also it would be nice to support displaying 'Basin / area' polygons over time instead of nodes. @Huite is that something that UGRID supports?