search

Deltares / Ribasim-NL

Ribasim water resources modeling in the Netherlands
https://ribasim.nl/
MIT License
4 stars 0 forks source link

concat_models #49

Closed d2hydro closed 8 months ago

d2hydro commented 9 months ago

fixes #48

Works with: https://github.com/Deltares/Ribasim/blob/e88ad64d225dd1db58847c05e7c2e2c203c325c7/python/ribasim/tests/test_model.py#L150

d2hydro commented 9 months ago

@visr, some things to consider solving in the Model.merge_model method (https://github.com/Deltares/Ribasim-NL/blob/239d638db0d66d977a2b9684152482d3e6a845a0/src/ribasim_nl/ribasim_nl/concat.py#L56-L61)?

SouthEndMusic commented 9 months ago

@d2hydro nice that you are already using model merging. However, why do you need the above index modifications? In Ribasim python we don't use those indices anywhere

d2hydro commented 9 months ago

@d2hydro nice that you are already using model merging. However, why do you need the above index modifications? In Ribasim python we don't use those indices anywhere

@SouthEndMusic, I read and merge a list of models iteratively. Issues arise while writing the merged model to filepath. Two issues arise:

  1. One model has a column 'node_id', the others not. The merged model has has nan-values in the column node_id. When with 'mode.write(filepath)' the validator gives a validation-error on validing the node-id column (due to nan-values)
  2. If 1 is fixed, pyogrio doesn't want to write edges as the index in edges is not unique. That is correct; model.network.edge.df.index.duplicated().any() gives True.

You can reproduce the above behavior with these two models: https://we.tl/t-sscLwd3Vhw

SouthEndMusic commented 9 months ago

One model has a column 'node_id', the others not. The merged model has has nan-values in the column node_id. When with 'mode.write(filepath)' the validator gives a validation-error on validing the node-id column (due to nan-values) If 1 is fixed, pyogrio doesn't want to write edges as the index in edges is not unique. That is correct; model.network.edge.df.index.duplicated().any() gives True.

You can reproduce the above behavior with these two models: https://we.tl/t-sscLwd3Vhw

Thanks for the response. I fixed the second point (in an upcoming commit), but I'm unsure about the first. node_id is not a standard column of the Node table, not sure why that doesn't raise validation errors. The error you mentioned is still triggered because the method which extracts node IDs from a table only looks for a node_id column , even if it shouldn't be there.

Since https://github.com/Deltares/Ribasim/pull/794 extra columns are prefixed with meta_, but maybe that does not happen when a model is read into ribasim Python from disk. @evetion?

SouthEndMusic commented 9 months ago

@d2hydro the first problem will not be fixed in my PR, I made a new issue: https://github.com/Deltares/Ribasim/issues/934.

evetion commented 9 months ago

One model has a column 'node_id', the others not. The merged model has has nan-values in the column node_id. When with 'mode.write(filepath)' the validator gives a validation-error on validing the node-id column (due to nan-values)

I can't replicate this validation error on writing. Note that with the new PR, the node_id will be renamed meta_node_id automatically.

d2hydro commented 9 months ago

@SouthEndMusic and @evetion, that is all OK, and I am looking forward to the meta_{colomn_name} implementation. Is it possible to merge #ribasim:914 and release it to a higher ribasim-version asap? I want to update the toml in this branch before merging to main so that the concat-function works.

evetion commented 9 months ago

@SouthEndMusic and @evetion, that is all OK, and I am looking forward to the meta_{colomn_name} implementation. Is it possible to merge #ribasim:914 and release it to a higher ribasim-version asap? I want to update the toml in this branch before merging to main so that the concat-function works.

We're working on that now, I expect a new release tomorrow.

SouthEndMusic commented 9 months ago

@d2hydro I can now merge and write your models without problems on the merging_models branch due to the latest fix by @evetion:

model1 = ribasim.Model.read(datadir / "HHNK_Ribasim/HHNK.toml")
model2 = ribasim.Model.read(datadir / "rijkswateren/ribasim.toml")
model1.smart_merge(model2)

model1.write(datadir / "merged/ribasim.toml")
d2hydro commented 9 months ago 
smart_merge

@SouthEndMusic and @evetion, in concat I give every merged model Node and Edge some extra attributes (for now waterbeheerder and zoom_level). I have to make that meta_waterbeheerder and meta_zoom_level in order not to crash:

https://github.com/Deltares/Ribasim-NL/blob/3d53e586d762ba6d5153ec8513e733390cdd934d/src/ribasim_nl/ribasim_nl/concat.py#L22-L27

I don't know if you want to fix this, but this will give an Exception:

model1 = ribasim.Model.read(datadir / "HHNK_Ribasim/HHNK.toml")
model1.network.node.df["foo"] = "bar"
model2 = ribasim.Model.read(datadir / "rijkswateren/ribasim.toml")
model2.network.node.df["foo"] = "bar"

model1.smart_merge(model2)