-
Notifications
You must be signed in to change notification settings - Fork 8
Merge boundary forcings for quantity's that share the same bc file. #993
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,12 +2,13 @@ | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||
| from typing import Any, Dict, Optional, Tuple, Union | ||||||||||||||||||||||||||||||||||
| from typing import Any, Dict, List, Optional, Tuple, Union | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from tqdm import tqdm | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| from hydrolib.core.base.file_manager import PathOrStr | ||||||||||||||||||||||||||||||||||
| from hydrolib.core.base.utils import PathStyle | ||||||||||||||||||||||||||||||||||
| from hydrolib.core.dflowfm.bc.models import ForcingModel | ||||||||||||||||||||||||||||||||||
| from hydrolib.core.dflowfm.ext.models import ( | ||||||||||||||||||||||||||||||||||
| Boundary, | ||||||||||||||||||||||||||||||||||
| ExtModel, | ||||||||||||||||||||||||||||||||||
|
|
@@ -430,6 +431,7 @@ def save(self, backup: bool = True, recursive: bool = True): | |||||||||||||||||||||||||||||||||
| num_quantities_inifield = len(self.inifield_model.parameter) + len( | ||||||||||||||||||||||||||||||||||
| self.inifield_model.initial | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
| self._merge_boundaries() | ||||||||||||||||||||||||||||||||||
| if num_quantities_inifield > 0: | ||||||||||||||||||||||||||||||||||
| self._save_inifield_model(backup, recursive) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -457,6 +459,60 @@ def save(self, backup: bool = True, recursive: bool = True): | |||||||||||||||||||||||||||||||||
| self.mdu_parser.clean() | ||||||||||||||||||||||||||||||||||
| self.mdu_parser.save(backup=backup) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| def _merge_boundaries(self): | ||||||||||||||||||||||||||||||||||
| """Merge boundary conditions that have the same .bc file filepath property. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| When writing a quantity to a .bc file, if the .bc file already exists in another boundary condition, | ||||||||||||||||||||||||||||||||||
| the new quantity should be added to the existing .bc file instead of creating a new one. | ||||||||||||||||||||||||||||||||||
| This method merges the forcings from quantities with the same .bc file filepath property. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Example: | ||||||||||||||||||||||||||||||||||
| A simplified example of merging boundary conditions with the same .bc file filepath property. | ||||||||||||||||||||||||||||||||||
| - Before merging: | ||||||||||||||||||||||||||||||||||
| boundary: | ||||||||||||||||||||||||||||||||||
| - quantity1 | ||||||||||||||||||||||||||||||||||
| - forcingfile: | ||||||||||||||||||||||||||||||||||
| filepath: 'boundary1.bc' | ||||||||||||||||||||||||||||||||||
| forcing: [quantity1] | ||||||||||||||||||||||||||||||||||
| boundary: | ||||||||||||||||||||||||||||||||||
| - quantity2 | ||||||||||||||||||||||||||||||||||
| - forcingfile: | ||||||||||||||||||||||||||||||||||
| filepath: 'boundary1.bc' | ||||||||||||||||||||||||||||||||||
| forcing: [quantity2] | ||||||||||||||||||||||||||||||||||
| - After merging: | ||||||||||||||||||||||||||||||||||
| boundary: | ||||||||||||||||||||||||||||||||||
| - quantity1 | ||||||||||||||||||||||||||||||||||
| - forcingfile: | ||||||||||||||||||||||||||||||||||
| filepath: 'boundary1.bc' | ||||||||||||||||||||||||||||||||||
| forcing: [quantity1, quantity2] | ||||||||||||||||||||||||||||||||||
| boundary: | ||||||||||||||||||||||||||||||||||
| - quantity2 | ||||||||||||||||||||||||||||||||||
| - forcingfile: | ||||||||||||||||||||||||||||||||||
| filepath: 'boundary1.bc' | ||||||||||||||||||||||||||||||||||
| forcing: [quantity1, quantity2] | ||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||
| merged_boundaries: Dict[Path, ForcingModel] = {} | ||||||||||||||||||||||||||||||||||
| for boundary in self.ext_model.boundary: | ||||||||||||||||||||||||||||||||||
| if not isinstance(boundary.forcingfile, list): | ||||||||||||||||||||||||||||||||||
| boundary.forcingfile = [boundary.forcingfile] | ||||||||||||||||||||||||||||||||||
| for forcingfile in boundary.forcingfile: | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+496
to
+498
|
||||||||||||||||||||||||||||||||||
| 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: |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type signature allows
ForcingModelas a single value butList[Union[ForcingModel, DiskOnlyFileModel]]as a list. This asymmetry is inconsistent - the single value case doesn't allowDiskOnlyFileModel. Consider changing toUnion[ForcingModel, DiskOnlyFileModel, List[Union[ForcingModel, DiskOnlyFileModel]]]for consistency.