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

Merging models #914

Closed
wants to merge 33 commits into from
Closed

Merging models #914

wants to merge 33 commits into from

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Dec 20, 2023

Fixes #912.

The general design is as in the above mentioned issue. 2 things that are kept in mind when merging model B into model A:

  • B itself should not be modified, so a copy of it is made and this copy is modified. This modified B is then passed on to merge methods on node and table level;
  • Only the merge method on the model level makes modifications, not the ones on node and table level. Only the table merge method does asserts for node ID conflicts (so allocation network IDs are not checked, as these are not unique. Also node/edge geometries are not checked).

@SouthEndMusic SouthEndMusic marked this pull request as draft December 20, 2023 11:59
@SouthEndMusic
Copy link
Collaborator Author

@evetion mypy still complains about a few things, I don't know what the best way would be to fix them.

@SouthEndMusic SouthEndMusic marked this pull request as ready for review December 21, 2023 18:58
@@ -1065,3 +1071,123 @@ def allocation_example_model():
)

return model


def main_network_with_subnetworks_model():
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll pick up this model in #851 for testing.

python/ribasim/ribasim/geometry/edge.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/geometry/edge.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/geometry/edge.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/geometry/node.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/input_base.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/model.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/model.py Outdated Show resolved Hide resolved
self.network.edge.plot(ax=ax, zorder=2)
self.plot_control_listen(ax)
self.network.node.plot(ax=ax, zorder=3)

ax.legend(loc="lower left", bbox_to_anchor=(1, 0.5))
handles, labels = ax.get_legend_handles_labels()
Copy link
Member

Choose a reason for hiding this comment

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

This should ideally not be part of the PR (but will ignore it here)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean the whole subnetwork plotting thing? I guess it would have bean cleaner if that was done in a separate PR, but it did help with debugging of the model merging

python/ribasim/tests/test_model.py Show resolved Hide resolved
python/ribasim/tests/test_model.py Outdated Show resolved Hide resolved
@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented Jan 9, 2024

To do:

  • Process comments by Maarten above
  • Remove Model.merge options: offset_node_id and offset_allocation_network_id are always computed
  • offset_spatial: make functions in tests
  • inplace: don't make copy of entire model, only loop over nodes and modify those
  • concat_models Ribasim-NL#49

@SouthEndMusic SouthEndMusic requested review from evetion and visr January 11, 2024 12:17
@SouthEndMusic SouthEndMusic marked this pull request as draft January 11, 2024 13:07
@SouthEndMusic SouthEndMusic marked this pull request as ready for review January 11, 2024 13:23
@visr visr marked this pull request as draft January 12, 2024 13:34
@evetion
Copy link
Member

evetion commented Jan 16, 2024

  • Remove special code to merge initial conditions (default states, after it's removed by another PR)
  • Make merge methods on lower levels (node, tables) private
  • Make use of model.copy() and check whether it is needed everywhere (don't use a keyword deep=True, just do the right thing)
  • Make inplace merge private (only merge (which copies) on model level)

Copy link

sonarcloud bot commented Jan 18, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@visr
Copy link
Member

visr commented Feb 14, 2024

This is paused, after #1110 we can decide how to proceed with this.

@visr
Copy link
Member

visr commented Mar 19, 2024

Closing this PR, since so many things changed that it probably needs a new start. The PR can still be used as inspiration for #912.

@visr visr closed this Mar 19, 2024
@visr visr deleted the merging_models branch March 19, 2024 20:08
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.

Ability to merge two Ribasim models in Python
3 participants