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

Source priority input #2022

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Source priority input #2022

wants to merge 9 commits into from

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Jan 29, 2025

Fixes #1505.

To do:

  • Update QGIS plugin based on table changes
  • Allow for setting source priority with add API and/or separate function
  • Process the source priority data in the core
  • Make column renaming priority -> demand_priority a non-breaking migration step

@SouthEndMusic SouthEndMusic requested a review from evetion January 29, 2025 12:34
@SouthEndMusic SouthEndMusic marked this pull request as draft January 29, 2025 12:34
Comment on lines 200 to 206
for n in range(1, len(names_lowered) + 1):
node_name_snake_case = "_".join(names_lowered[:n])
if node_name_snake_case in node_names_snake_case:
node_name = "".join(names[:n])
table_name = "_".join(names_lowered[n:])
return node_name + delimiter + table_name
return names[0]
Copy link
Collaborator Author

@SouthEndMusic SouthEndMusic Jan 29, 2025

Choose a reason for hiding this comment

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

This doesn't work properly yet (in the sense that it doesn't produce a proper table name for my new table), but the python tests failed on this because my new table name neither contains a node name nor is it one word long

@SouthEndMusic SouthEndMusic added the breaking A change that breaks existing models label Jan 29, 2025
Copy link
Member

@evetion evetion left a comment

Choose a reason for hiding this comment

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

Can you describe the design in this PR? From what I remember we need a configurable priority per nodetype only? And if we needed to override that (not sure), there can be a column in the Node table.

Given that context, why do we need a python/ribasim/ribasim/allocation/source_priorities.py?

Comment on lines +26 to +30
default_source_priority_user_demand = 1000 # optional, default 1000
default_source_priority_boundary = 2000 # optional, default 2000
default_source_priority_level_demand = 3000 # optional, default 3000
default_source_priority_flow_demand = 4000 # optional, default 4000
default_source_priority_subnetwork_inlet = 5000 # optional, default 5000
Copy link
Member

Choose a reason for hiding this comment

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

I would make it a substructure here, called source_priority or similar, and removing default from the name.

@SouthEndMusic
Copy link
Collaborator Author

Can you describe the design in this PR?

@evetion It's in the issue: #1505

@evetion
Copy link
Member

evetion commented Jan 29, 2025

Can you describe the design in this PR?

@evetion It's in the issue: #1505

It's still nice to summarize what the changes are in a PR description (and later commit message). Can be very short, as in rename field to demand_field, adds table X, adds config option, especially given the number of files changed here.

@SouthEndMusic SouthEndMusic removed the breaking A change that breaks existing models label Jan 29, 2025
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.

Handling default and non-default source priorities in allocation
2 participants