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

upgrade 1D #390

Merged
merged 44 commits into from
Nov 25, 2024
Merged

upgrade 1D #390

merged 44 commits into from
Nov 25, 2024

Conversation

margrietpalm
Copy link
Contributor

@margrietpalm margrietpalm commented Oct 25, 2024

Major changes:

  • CrossSectionDefinition is no longer in a separate table but instead these definitions are in CrossSectionLocation, Pipe, Orifice, Culvert and Weir. Because the CrossSecitonDefinitions must be in the gridadmin, they are rebuilt based on the defintiions in the other tables. Only unique entries are kept they are mapped to their origin tables and rows. This also affects the order in which data is read from the schematisation in make_gridadmin.
  • The width and height of a tabulated cross section are no longer stored in the same column as those for a non-tabulated cross section but instead these are gathered in the cross_section_shape. This required some changes in tabulators and reading of width and height
  • There is no longer a difference between singular and plural vegetation parameters. The singular ones are kept and will only contain a single value. The plural ones are replaced by cross_section_vegetation_table. This is all handled in set_friction_vegetation_values.
  • The new column model_settings.node_open_water_detection is processed that modified how open / closed is detected for ConnectionNodes; either based on having storage (node_open_water_detection=1, default in migration) or being connected to channels (node_open_water_detection=0)
  • Obstacle is extended with columns: affects_2d, affects_1d2d_open_water and affects_1d2d_closed. When these are True, dpumax for intersected flow lines is increased with the obstacle's crest level. On migration affects_2d and affects_1d2d_open_water are set to True and affects_1d2d_closed to False which should cause no changes in behavior.
  • Culverts, Pipes, Orifices and Weirs can either have their friction parameters in the table or be linked to a material.

Testing

I used https://github.com/nens/schema_300_integration_tests/tree/test_228 to test these changes. The gridadmin files produced are not identical, but should be equivalent. See https://github.com/nens/schema_300_integration_tests/blob/test_228/test_libs/compare_gridadmin.py for details on this comparisson.

* Disable any code that cannot yet work
* Skip tests or asserts that are known to fail atm
Any node with bottom_level != -9999 is considered to be a manhole. For migrated
schematisations, this are all nodes that were linked to a manhole.
@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from 884020f to d6220d1 Compare October 29, 2024 09:28
…ne_id.

Any node with bottom_level != nan is considered to be a manhole. For migrated
schematisations, this are all nodes that were linked to a manhole.
Added attribute is_manhole to Node based on ConnectionNode.bottom_level != nan.
I did consider using Node.dmax because this is initialized as ConnectionNode.bottom_level.
But any modifications to that value may break this functionality.
…idth and height and add unit test for _parse_tabulated
@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from c214d46 to dc0cbe1 Compare November 5, 2024 10:08
Comment on lines +318 to +341
if test_case == "open":
obstacles.affects_1d2d_open_water = np.array([True, True, True])
obstacles.affects_1d2d_closed = np.array([False, False, False])
lines = Lines1D2D(
id=range(3),
kcu=[
LineType.LINE_1D2D_SINGLE_CONNECTED_OPEN_WATER,
LineType.LINE_1D2D_SINGLE_CONNECTED_CLOSED,
LineType.LINE_1D2D_SINGLE_CONNECTED_OPEN_WATER,
],
)
lines.assign_dpumax_from_obstacles_open(obstacles)
elif test_case == "closed":
obstacles.affects_1d2d_closed = np.array([True, True, True])
obstacles.affects_1d2d_open_water = np.array([False, False, False])
lines = Lines1D2D(
id=range(3),
kcu=[
LineType.LINE_1D2D_SINGLE_CONNECTED_CLOSED,
LineType.LINE_1D2D_SINGLE_CONNECTED_OPEN_WATER,
LineType.LINE_1D2D_SINGLE_CONNECTED_CLOSED,
],
)
lines.assign_dpumax_from_obstacles_closed(obstacles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions of doing this in a better way are welcome. I've considered putting this in a parameterize decorator, but that became complex very quickly.

@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from 9d9c5ea to 114cb67 Compare November 7, 2024 09:13
Comment on lines +105 to +118
locations = db.get_cross_section_locations()
pipes = db.get_pipes()
weirs = db.get_weirs()
culverts = db.get_culverts()
orifices = db.get_orifices()
cross_section_definitions = db.get_cross_section_definitions()
(
cross_section_definitions_unique,
mapping,
) = cross_section_definitions.get_unique()
map_cross_section_definition(
[locations, orifices, pipes, culverts, weirs], mapping
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading of cross section locations, pipes, weirs, culverts and orifices is moved up so the extracted cross section definitions can be properly mapped.

@margrietpalm margrietpalm changed the title WIP: upgrade 1D upgrade 1D Nov 13, 2024
@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from 4e00db7 to 11136ad Compare November 18, 2024 14:37
@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from 11136ad to ba1bc99 Compare November 18, 2024 14:42
def is_channel(self, content_pk, channels):
"""Whether object is connected to a channel"""
has_channel = np.logical_or(
np.isin(self.id, channels.connection_node_start_id),
Copy link
Collaborator

@martijn-siemerink martijn-siemerink Nov 18, 2024

Choose a reason for hiding this comment

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

Ik heb nog wel vraagteken bij deze check. Wat nu als de connection_node_start_id ook aan een pipe zit? Dan is dat nu altijd een channel, ondanks dat het misschien een manhole is en is_closed logischer is. Ik overzie het niet helemaal, maar potentieel is dit te strict.

als kcu bekend is hier voor aanliggende 1d2d verbinding zou ik die checken, ook omdat de dpumax uiteindelijk gezet wordt voor de 1d2d line.

LineType enum definieert de kcu types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dit lijkt me niet de plek om zoiets aan te passen; dit doet namelijk niet meer dan checken of iets een channel is. Als dat de lading niet dekt moet hooguit de naam worden aangepast.

Ik overzie het niet helemaal, maar potentieel is dit te strict. Als kcu bekend is hier voor aanliggende 1d2d verbinding zou ik die checken, ook omdat de dpumax uiteindelijk gezet wordt voor de 1d2d line.

Lijkt me iets wat aangepast moet worden in het kiezen tussen is_closed en is_channel: https://github.com/nens/threedigrid-builder/blob/margriet_389_schema_300_1D/threedigrid_builder/grid/grid.py#L757-L765. Maar ik snap nog niet helemaal welke logica je hier zou willen zien. Is dat iets als:

if node_open_water_detection == 0 and isinstance(objects, ConnectionNodes):
  if line.kcu != -9999:
      is_closed = (line.kcu == LINE_1D2D_SINGLE_CONNECTED_CLOSED or line.kcu == INE_1D2D_DOUBLE_CONNECTED_CLOSED)
  else:
      is_closed = is_channel()

self.write_dataset(group, "manhole_id", nodes.manhole_id)
# Set manhole_id to match nodes.id when there is a manhole
self.write_dataset(
group, "manhole_id", np.where(nodes.is_manhole, nodes.id + 1, -9999)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Als het originele manhole_id de database pk is, zou ik hier de connection_node pk schrijven. Anders prima

Copy link
Contributor Author

Choose a reason for hiding this comment

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

De originele manhole_id zit niet meer in de schematisatie; dus die kan ook niet meer in de gridadmin terecht komen. Daarnaast lijkt het me vreemd om een id te gebruiken dat niet meer naar een bestaand object refereert.

@margrietpalm margrietpalm force-pushed the margriet_389_schema_300_1D branch from c2ad621 to 4e3dbf0 Compare November 25, 2024 11:23
@margrietpalm margrietpalm merged commit 0cc5668 into master Nov 25, 2024
9 checks passed
@margrietpalm margrietpalm deleted the margriet_389_schema_300_1D branch November 25, 2024 12:12
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.

2 participants