Skip to content

Combine layers unstructured#426

Merged
rubencalje merged 15 commits intodevfrom
combine_layers_unstructured
Mar 18, 2025
Merged

Combine layers unstructured#426
rubencalje merged 15 commits intodevfrom
combine_layers_unstructured

Conversation

@dbrakenhoff
Copy link
Collaborator

Allow combining layers in vertex datasets.

Add methods to check dataset or dataarray type:

  • nlmod.grid.is_structured(ds)
  • nlmod.grid.is_vertex(ds)
  • nlmod.grid.is_layered(ds)

Added a GridTypeDims enum that specifies the dimensions for different grid types. Perhaps this and the above methods can be used across nlmod for consistency in how we check for different types of inputs. But that is best left to another PR if we agree this is the way to go.

- is_structured
- is_vertex
- is_layered
- add method to get empty data array with new layer coord but maintaining other dims/coords from source data array
- only drop datavars with layer coord
- add the other datavars back to final combined result
- lazy formatting in logging
- specificies dimensions for grid types
- no longer add reindexer to attributes (this was annoying when saving the dataset)
@github-project-automation github-project-automation bot moved this to Todo in NHFLO Mar 13, 2025
@dbrakenhoff dbrakenhoff requested review from OnnoEbbens and rubencalje and removed request for OnnoEbbens March 13, 2025 15:27
@dbrakenhoff dbrakenhoff self-assigned this Mar 13, 2025
@dbrakenhoff dbrakenhoff added the enhancement New feature or request label Mar 13, 2025
@dbrakenhoff
Copy link
Collaborator Author

test fail related to codacy api outage: https://status.codacy.com/

Copy link
Collaborator

@rubencalje rubencalje left a comment

Choose a reason for hiding this comment

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

Looks good. The use of Enum looks a bit complicated to me, but maybe you can show me what is the benefit of this at a later time.

@github-project-automation github-project-automation bot moved this from Todo to In Progress in NHFLO Mar 14, 2025
@dbrakenhoff
Copy link
Collaborator Author

dbrakenhoff commented Mar 14, 2025

Looks good. The use of Enum looks a bit complicated to me, but maybe you can show me what is the benefit of this at a later time.

Good question. My reasoning so far: it's fancy, and it autocompletes nicely in your editor across files. It has a nice representation:

<GridTypeDims.STRUCTURED: ('y', 'x')>.

You can do a reverse lookup, like this:

nlmod.grid.GridTypeDims(da.dims) is nlmod.grid.GridTypeDims.STRUCTURED

Or even nicer:

class GridTypeDims(Enum):
    """Enum for grid dimensions."""

    STRUCTURED_LAYERED = ("layer", "y", "x")
    VERTEX_LAYERED = ("layer", "icell2d")
    STRUCTURED = ("y", "x")
    VERTEX = ("icell2d",)

    @classmethod
    def parse_dims(cls, ds):
        """Get GridTypeDim from dataset or dataarray.

        Parameters
        ----------
        ds : xr.Dataset or xr.DataArray
            Dataset or DataArray to parse.
        """
        for gridtype in GridTypeDims:
            if set(gridtype.value).issubset(ds.dims):
                return gridtype

Which allows

GridTypeDims.parse_dims(da) in (GridTypeDims.VERTEX, GridTypeDims.VERTEX_LAYERED)

or

GridTypeDims.parse_dims(da) is GridTypeDims.VERTEX_LAYERED)

- add parse_dims class method
- use this in is_structured and is_vertex grid-methods
Copy link
Collaborator

@OnnoEbbens OnnoEbbens left a comment

Choose a reason for hiding this comment

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

Nice improvement! I also didn't know about the enum inheritance but it looks nice. I think we should implement this for all the gridtype checks. We can maybe even remove the gridtype attribute.

Especially looking forward to replace this code in the resample module for a decent gridtype check:
if len(da.shape) > 1 and len(da.y) == da.shape[-2] and len(da.x) == da.shape[-1]

@rubencalje rubencalje merged commit 58d8c40 into dev Mar 18, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in NHFLO Mar 18, 2025
@rubencalje rubencalje deleted the combine_layers_unstructured branch March 18, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants