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

merge_partitions fails for datasets with multiple topologies #134

Closed
veenstrajelmer opened this issue Aug 3, 2023 · 3 comments · Fixed by #216
Closed

merge_partitions fails for datasets with multiple topologies #134

veenstrajelmer opened this issue Aug 3, 2023 · 3 comments · Fixed by #216
Assignees

Comments

@veenstrajelmer
Copy link
Collaborator

Merging partitions with multiple topologies (1D2D model) raises: "ValueError: Expected 4 UGRID topologies for 4 partitions, received: [...]"

MWE:

import glob
import xugrid as xu

file_nc = r'p:\1230882-emodnet_hrsm\GTSMv5.0\SO_NHrivGTSM\computations\BD014_fix_mapformat4\output\gtsm_model_0*_map.nc'
file_nc_list = glob.glob(file_nc)
partitions = []
for iF, file_nc_one in enumerate(file_nc_list):
    print(iF+1,end=' ')
    uds_part = xu.open_dataset(file_nc_one)
    partitions.append(uds_part)
uds = xu.merge_partitions(partitions)
@JoerivanEngelen
Copy link
Contributor

JoerivanEngelen commented Jan 31, 2024

Debugged this, it appears the issue stems from the fact that the 1D grid only occurs in one partition, whereas the 2D grid occurs in all four partitions.

What would be appropriate behavior here @Huite : Be strict, as is the current behavior, or allow Datasets with a mix of 2D grids as well as 1D grid in some partitions?

@veenstrajelmer
Copy link
Collaborator Author

For visual reference, this issue contains an image of the grid: Deltares/dfm_tools#497

@Huite
Copy link
Collaborator

Huite commented Jan 31, 2024

Good question. It seems reasonable to allow this.

The check is easy to adapt by also checking for length 1:

    n = n_partition
    if not all(len(v) == n or len(v) == 1 for v in grouped.values()):
        raise ValueError(
            f"Expected {n} UGRID topologies for {n} partitions, received: " f"{grouped}"
        )

Maybe everything already __ just __ works if you only remove the check?

Probably not, because the grid topologies are zipped with the data_objects. So if the single topology happens to be in the first partition, it'll work, but not if it's in the third.

@JoerivanEngelen JoerivanEngelen moved this from 🤝 Accepted to 🏗 In Progress in iMOD Suite Feb 9, 2024
@JoerivanEngelen JoerivanEngelen self-assigned this Feb 9, 2024
@JoerivanEngelen JoerivanEngelen moved this from 🏗 In Progress to 🧐 In Review in iMOD Suite Feb 13, 2024
Huite added a commit that referenced this issue Feb 14, 2024
Issue #134 merge partitions with inconsistent grids amongst partitions
@github-project-automation github-project-automation bot moved this from 🧐 In Review to ✅ Done in iMOD Suite Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants