Merge boundary forcings for quantity's that share the same bc file. - #993
jeroen-deltares wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements boundary forcing merging to prevent data loss during boundary conversions. When multiple boundary conditions reference the same .bc file, their forcings are now consolidated into a single shared ForcingModel instead of being kept separate.
Key Changes
- Added
_merge_boundaries()method to consolidate forcings that share the same .bc filepath - Updated
Boundary.forcingfiletype annotation to supportDiskOnlyFileModelin addition toForcingModel
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hydrolib/tools/extforce_convert/main_converter.py | Implements the _merge_boundaries() method and calls it during save operations to merge forcings with identical .bc filepaths |
| hydrolib/core/dflowfm/ext/models.py | Updates the forcingfile field type to allow DiskOnlyFileModel alongside ForcingModel in lists |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for forcingfile in boundary.forcingfile: | ||
| bc_filepath = forcingfile.filepath | ||
| if bc_filepath in merged_boundaries: | ||
| merged_boundaries[bc_filepath].forcing.extend(forcingfile.forcing) |
There was a problem hiding this comment.
Direct extension of the forcing list can lead to duplicate entries if the same boundary is processed multiple times or if forcings are already present. Consider checking for duplicates before extending or using a set-based approach to ensure uniqueness of forcing entries.
| merged_boundaries[bc_filepath].forcing.extend(forcingfile.forcing) | |
| # Merge forcings, ensuring uniqueness and preserving order | |
| existing_forcings = merged_boundaries[bc_filepath].forcing | |
| new_forcings = forcingfile.forcing | |
| seen = set() | |
| unique_forcings = [] | |
| for f in existing_forcings + new_forcings: | |
| if f not in seen: | |
| unique_forcings.append(f) | |
| seen.add(f) | |
| merged_boundaries[bc_filepath].forcing = unique_forcings |
| for forcingfile in boundary.forcingfile: | ||
| bc_filepath = forcingfile.filepath | ||
| if forcingfile not in forcing_list: | ||
| if bc_filepath in merged_boundaries: | ||
| forcing_list.append(merged_boundaries[bc_filepath]) | ||
| else: | ||
| forcing_list.append(forcingfile) |
There was a problem hiding this comment.
The identity check forcingfile not in forcing_list may not work as intended because it compares object references rather than the filepath property. If the same filepath has multiple ForcingModel instances, they won't be detected as duplicates. Consider comparing bc_filepath values instead or using a set to track already-added filepaths.
| for forcingfile in boundary.forcingfile: | |
| bc_filepath = forcingfile.filepath | |
| if forcingfile not in forcing_list: | |
| if bc_filepath in merged_boundaries: | |
| forcing_list.append(merged_boundaries[bc_filepath]) | |
| else: | |
| forcing_list.append(forcingfile) | |
| seen_filepaths = set() | |
| for forcingfile in boundary.forcingfile: | |
| bc_filepath = forcingfile.filepath | |
| if bc_filepath not in seen_filepaths: | |
| if bc_filepath in merged_boundaries: | |
| forcing_list.append(merged_boundaries[bc_filepath]) | |
| else: | |
| forcing_list.append(forcingfile) | |
| seen_filepaths.add(bc_filepath) |
| default_factory=lambda: DiskOnlyFileModel(None), alias="locationFile" | ||
| ) | ||
| forcingfile: Union[ForcingModel, List[ForcingModel]] = Field(alias="forcingFile") | ||
| forcingfile: Union[ForcingModel, List[Union[ForcingModel, DiskOnlyFileModel]]] = ( |
There was a problem hiding this comment.
The type signature allows ForcingModel as a single value but List[Union[ForcingModel, DiskOnlyFileModel]] as a list. This asymmetry is inconsistent - the single value case doesn't allow DiskOnlyFileModel. Consider changing to Union[ForcingModel, DiskOnlyFileModel, List[Union[ForcingModel, DiskOnlyFileModel]]] for consistency.
| forcingfile: Union[ForcingModel, List[Union[ForcingModel, DiskOnlyFileModel]]] = ( | |
| forcingfile: Union[ForcingModel, DiskOnlyFileModel, List[Union[ForcingModel, DiskOnlyFileModel]]] = ( |
| if not isinstance(boundary.forcingfile, list): | ||
| boundary.forcingfile = [boundary.forcingfile] | ||
| for forcingfile in boundary.forcingfile: |
There was a problem hiding this comment.
The mutation of boundary.forcingfile from a single value to a list affects the original object and may have unintended side effects elsewhere in the codebase. Consider creating a local variable to hold the list representation instead of modifying the boundary object during the merge operation.
| if not isinstance(boundary.forcingfile, list): | |
| boundary.forcingfile = [boundary.forcingfile] | |
| for forcingfile in boundary.forcingfile: | |
| forcingfiles = boundary.forcingfile if isinstance(boundary.forcingfile, list) else [boundary.forcingfile] | |
| for forcingfile in forcingfiles: |
|
Description
Add a merging method for forcings to prevent loss of data in boundary conversions
Possibly could go wrong when attempting to merge
DiskOnlyFileModewithForcingModel.Attempt 2 see old PR: Multiple quantities in same bc file #992
Fixes polyline quantity is not properly moved to the new bc file #978
Type of change
Check relevant points.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration
Test A
Test B
Checklist:
Prepare items below using:
[ ❌ ] (markdown:
[ :x: ]) for TODO items[ ✅ ] (markdown:
[ :white_check_mark: ]) for DONE items[ N/A ] for items that are not applicable for this PR.