Fix unwanted dimensions in dataset #60

ealerskans · 2025-02-04T07:18:45Z

Describe your changes

This PR fixes a bug in what dimensions are added to the output dataset. Currently, the dimensions of the input dataset are all added. However, if one only wants to extract e.g. u and v from the 100 m altitude in the DANRA height levels dataset, the current implementation will add altitude as a dimension meaning that all selected variables will be broadcasted to have altitude as a dimension, even though we're only selecting one variables from one altitude. This is fixed by removing the coordinates assignment to the output dataset when initialising it.

Furthermore, there is a similar bug for the derived variables, in which the input dataset dimensions are used. The change by @joeloskarsson (joeloskarsson@8040378) fixes this by using the expected input variable dimensions instead as target_dims.

No change in dependencies for this fix.

Issue Link

Tries to solve #58.

Type of change

🐛 Bug fix (non-breaking change that fixes an issue)
✨ New feature (non-breaking change that adds functionality)
💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
I have performed a self-review of my code
For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
I have updated the documentation to cover introduced code changes
I have added tests that prove my fix is effective or that my feature works
I have given the PR a name that clearly describes the change, written in imperative form (context).
I have requested a reviewer and an assignee (assignee is responsible for merging)

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

the code is readable
the code is well tested
the code is documented (including return types and parameters)
the code is easy to maintain

Author checklist after completed review

I have added a line to the CHANGELOG describing this change, in a section
reflecting type of change (add section where missing):
- added: when you have added new functionality
- changed: when default behaviour of the code has been changed
- fixes: when your contribution fixes a bug

Checklist for assignee

PR is up to date with the base branch
the tests pass
author has added an entry to the changelog (and designated the change as added, changed or fixed)
Once the PR is ready to be merged, squash commits and merge the PR.

observingClouds

Thanks for the PR @ealerskans! Could you please add some tests, e.g. that the output variables are the one selected and that there are no NaN values?

- Test that the output variables match the ones specified in the config - Test that the output dataset does not contain any nans

ealerskans · 2025-02-06T13:14:24Z

Thanks for the PR @ealerskans! Could you please add some tests, e.g. that the output variables are the one selected and that there are no NaN values?

@observingClouds I tried to add some tests for what you suggested. I am very new to this so please have a look and let me know if something does not look right or it it would be better to do this in another way :)

…config

ealerskans · 2025-02-07T06:36:00Z

tests/test_dataset.py

+INVALID_PRESSURE_LEVEL_TEST_SECTION = """\
+inputs:
+  danra_pressure_levels:
+    path: https://object-store.os-api.cci1.ecmwf.int/mllam-testdata/danra_cropped/v0.2.0/pressure_levels.zarr
+    dims: [time, x, y, pressure]
+    variables:
+      z:
+        pressure:
+          values: [1000,]
+          units: hPa
+      t:
+        pressure:
+          values: [800, ]
+          units: hPa
+    dim_mapping:
+      time:
+        method: rename
+        dim: time
+      state_feature:
+        method: stack_variables_by_var_name
+        dims: [pressure]
+        name_format: "{var_name}{pressure}m"
+      grid_index:
+        method: stack
+        dims: [x, y]
+    target_output_variable: state
+"""


This part here is related to issue #61. We agreed that we should be able to support this kind of selection, where we select variable form different levels. But this is currently not supported and would require re-writing of the code. So not sure if this PR should be merged before that has been addressed. I am in process of implementing this and will hopefully create a PR for it later today.

Actually, I am now in doubt if this would even work after that change since that will introduce a check in the reading of the config file and then throw an error if we're trying to make this kind of selection.

This is a tricky question. I would still implement all tests and then add a decorator to the test that will allow it to be skipped in case a NotImplementedError is raised, like here but specific to NotImplementedError

As agreed, I will implement the decorator addition in #65 instead :)

observingClouds · 2025-02-13T08:18:53Z

blocked by #66 / #67

leifdenby · 2025-02-13T14:58:23Z

no longer blocked now that @observingClouds has merged #66 and #67 🥳

Refactor the config modifier to make it more general and make the file more structured

observingClouds

Sweet! This looks good to me now 😉

Apply fix 8040378 from joeloskarsson

7a7fc0e

ealerskans requested a review from observingClouds February 4, 2025 07:18

ealerskans assigned leifdenby Feb 4, 2025

observingClouds reviewed Feb 4, 2025

View reviewed changes

observingClouds linked an issue Feb 4, 2025 that may be closed by this pull request

Issue when adding coordinates to the output dataset #58

Closed

ealerskans mentioned this pull request Feb 5, 2025

Selection of variables by coord values results in missing/extra output variables with nans #61

Open

ealerskans added 2 commits February 6, 2025 12:57

Merge branch 'main' into bugfix/unwanted_dimensions

16dd0e6

Add tests

82f8e59

- Test that the output variables match the ones specified in the config - Test that the output dataset does not contain any nans

Better name for function modifying the inputs section in the example …

ecd3e6e

…config

ealerskans commented Feb 7, 2025

View reviewed changes

leifdenby added the bug Something isn't working label Feb 10, 2025

observingClouds added the blocked PR blocked by an other issue/PR label Feb 13, 2025

leifdenby removed the blocked PR blocked by an other issue/PR label Feb 13, 2025

observingClouds and others added 3 commits February 13, 2025 20:31

Merge branch 'main' into bugfix/unwanted_dimensions

38f287e

Pr/60 (#2)

e379bb8

Refactor the config modifier to make it more general and make the file more structured

Update CHANGELOG

827bfb1

observingClouds approved these changes Feb 14, 2025

View reviewed changes

observingClouds assigned observingClouds and unassigned leifdenby Feb 14, 2025

observingClouds merged commit 144ca10 into mllam:main Feb 14, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix unwanted dimensions in dataset #60

Fix unwanted dimensions in dataset #60

Uh oh!

ealerskans commented Feb 4, 2025 •

edited by observingClouds

Loading

Uh oh!

observingClouds left a comment

Uh oh!

ealerskans commented Feb 6, 2025

Uh oh!

ealerskans Feb 7, 2025

Uh oh!

ealerskans Feb 7, 2025

Uh oh!

observingClouds Feb 7, 2025

Uh oh!

ealerskans Feb 7, 2025

Uh oh!

observingClouds commented Feb 13, 2025

Uh oh!

leifdenby commented Feb 13, 2025

Uh oh!

observingClouds left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix unwanted dimensions in dataset #60

Fix unwanted dimensions in dataset #60

Uh oh!

Conversation

ealerskans commented Feb 4, 2025 • edited by observingClouds Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Issue Link

Type of change

Checklist before requesting a review

Checklist for reviewers

Author checklist after completed review

Checklist for assignee

Uh oh!

observingClouds left a comment

Choose a reason for hiding this comment

Uh oh!

ealerskans commented Feb 6, 2025

Uh oh!

ealerskans Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

ealerskans Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

observingClouds Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

ealerskans Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

observingClouds commented Feb 13, 2025

Uh oh!

leifdenby commented Feb 13, 2025

Uh oh!

observingClouds left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ealerskans commented Feb 4, 2025 •

edited by observingClouds

Loading