Specifying vertical levels rather than height levels #2063

lambert-p · 2024-12-09T13:42:33Z

Description:

I have changed all variables named height_levels to vertical_level. This is to make the setting of vertical levels more transparent with the previous height_level referring to height, or pressure levels if needed (using a boolean pressure option). The vertical_level argument is therefore clearer to use.

Testing:

Ran tests and they passed OK
Edited tests for the new feature(s)

codecov · 2024-12-17T12:17:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.41%. Comparing base (84a8944) to head (c2f5ee0).
Report is 70 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2063      +/-   ##
==========================================
+ Coverage   98.39%   98.41%   +0.01%     
==========================================
  Files         124      135      +11     
  Lines       12212    13304    +1092     
==========================================
+ Hits        12016    13093    +1077     
- Misses        196      211      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

maxwhitemet

I have reviewed your code and highlight only very minor typo corrections. I will put the ticket into second review.

improver_tests/synthetic_data/test_set_up_test_cubes.py

improver/synthetic_data/set_up_test_cubes.py

lambert-p · 2024-12-19T12:54:48Z

The ticket mentions adding pressure/height flags to the synthetic data but I think this has already been done. #1993

Kat-90 · 2024-12-31T15:29:21Z

The ticket mentions adding pressure/height flags to the synthetic data but I think this has already been done. #1993

After reviewing the work, I think that this hasn't quite been done yet. Ben has mentioned that there is already a pressure flag to change from height to pressure which is what you've noticed too, and this was fine back when all the levels were called height.
What I think he is suggesting is that now we have a more generalised variable name or "vertical levels", we should have 2 flags, one for pressure AND one for heights. So rather than assuming if pressure = false then it sets heights, we set heights explicitly as we now have no mention of heights anywhere at all.
This would make things even more transparent.

Kat-90

I'm a little confused how the acceptance tests are passing at the moment. This PR updates the kgo names so they shouldn't be able to find them - these fail for me locally, plus some extra ones relating to cell methods from this PR: #2062

I think you need to update the names of any KGO files that you have changed in the acceptance tests. These need another PR in the imrpover_test_data repository

Kat-90 · 2024-12-19T09:38:49Z

improver_tests/synthetic_data/test_set_up_test_cubes.py

@@ -428,54 +433,54 @@ def test_error_unmatched_realizations(self):
                self.data_3d, realizations=np.arange(realizations_len)
            )

-    def test_error_unmatched_height_levels(self):
+    def test_error_unmatched_vertical_levels(self):
        """Test error is raised if the heights provided do not match the


I think this should docstring be changed to "levels" rather than heights

This still needs changing

This test actually needs to be duplicated. One named height levels, one named pressure levels. They are testing explicitly as the coordinate name is set in the set up function. So this test should be test_error_unmatched_height_levels as it was originally, with the height=True keyword argument provided. The copy should be called test_error_unmatched_pressure_levels, with pressure=True, and the msg tested will be modified to say `"Cannot generate {} pressures with data of length {}".

Kat-90 · 2024-12-31T16:12:32Z

improver/synthetic_data/set_up_test_cubes.py

        pressure:
-            Flag to indicate whether the height levels are specified as pressure, in Pa.
+            Flag to indicate whether the vertical levels are specified as pressure, in Pa.


I'd add another flag here for "height" and subsequent places that need it.

lambert-p · 2025-01-03T11:39:51Z

The kgo names have been changed and I have added a height flag as requested

Kat-90

Just a few more changes needed here and some minor docstring additions.
We discussed defaulting the new height flag to True but looking further into the code, this is already happening so I suggest we change it to False.

Kat-90 · 2025-01-08T08:18:50Z

improver/synthetic_data/set_up_test_cubes.py

@@ -420,6 +421,7 @@ def set_up_spot_variable_cube(
    realizations: Optional[Union[List[float], ndarray]] = None,
    vertical_levels: Optional[Union[List[float], ndarray]] = None,
    pressure: bool = False,
+    height: bool = True,


Need to add the height flag to this docstring below (line 468)

Kat-90 · 2025-01-08T08:23:58Z

improver/synthetic_data/set_up_test_cubes.py

@@ -420,6 +421,7 @@ def set_up_spot_variable_cube(
    realizations: Optional[Union[List[float], ndarray]] = None,
    vertical_levels: Optional[Union[List[float], ndarray]] = None,
    pressure: bool = False,
+    height: bool = True,


Have we considered what happens if pressure is set to True and the height flag is left unset? It's default is True so we would have 2 True coords which will likely fail. For this to work you'd probably have to write a bit of code to handle it.

On line 370 which is the function that uses the pressure and height flags, there is a bit of code that says if pressure is set (i.e. if pressure=True) then set pressure, else use height. For this reason it might be worth just setting the default flag for height to False as it's already defaulting to height within the code.

Kat-90 · 2025-01-08T08:29:20Z

improver/synthetic_data/set_up_test_cubes.py

@@ -539,6 +542,7 @@ def set_up_variable_cube(
    realizations: Optional[Union[List[float], ndarray]] = None,
    vertical_levels: Optional[Union[List[float], ndarray]] = None,
    pressure: bool = False,
+    height: bool = True,


As above, please add height to the docstring below the pressure flag. And consider changing height default to False.

Kat-90 · 2025-01-08T08:31:58Z

improver_tests/synthetic_data/test_set_up_test_cubes.py

@@ -428,54 +433,54 @@ def test_error_unmatched_realizations(self):
                self.data_3d, realizations=np.arange(realizations_len)
            )

-    def test_error_unmatched_height_levels(self):
+    def test_error_unmatched_vertical_levels(self):
        """Test error is raised if the heights provided do not match the


This still needs changing

Kat-90

One last thing.

improver/synthetic_data/set_up_test_cubes.py

Kat-90

Thanks, looks good to me now, passing to @bayliffe for a quick glance.

bayliffe

Thanks for tackling this Phoebe. I've put an upsetting number of comments on this, but that's largely to try and help make it clear what I want, rather than leaving you one comment and saying "repeat everywhere".

We should have a chat about this to ensure you understand the reasoning for what I've requested. Give me a call once you've read the review.

improver/synthetic_data/set_up_test_cubes.py

improver/synthetic_data/utilities.py

improver/cli/generate_metadata_cube.py

improver_tests/synthetic_data/test_set_up_test_cubes.py

bayliffe

Many fewer comments this time. Thanks for all the changes, nearly there with just a few to go.

improver/synthetic_data/set_up_test_cubes.py

improver/utilities/cube_manipulation.py

improver_tests/synthetic_data/test_generate_metadata.py

improver_tests/synthetic_data/test_utilities.py

improver_tests/utilities/cube_manipulation/test_maximum_in_height.py

improver_tests/wind_calculations/wind_components/test_ResolveWindComponents.py

lambert-p · 2025-02-11T10:53:53Z

Thanks for the comments Ben, I have made the changes and added the tests required.

bayliffe

The tiniest comment which I've largely included to demonstrate how you can reset a file from an older commit. If you make that change I will approve and merge all of this.

Thanks for all the work.

bayliffe · 2025-02-12T07:34:13Z

improver/utilities/cube_manipulation.py

@@ -791,7 +791,7 @@ def height_of_maximum(
            A cube of the maximum value over the height coordinate.
        find_lowest:
            If true then the lowest maximum height will be found (for cases where
-            there are two heights with the maximum vertical velocity.) Otherwise the highest
+            there are two heights with the maximum height velocity.) Otherwise the highest


We've nearly left this file unchanged. Last thing is that whilst we are talking about height levels, the velocity discussed is the velocity in the z-dimension, i.e. the vertical velocity. If you can put this back as shown here then this file will be exactly as it was before.

Suggested change

there are two heights with the maximum height velocity.) Otherwise the highest

there are two heights with the maximum vertical velocity.) Otherwise the highest

If in future you want to completely undo the changes to a file, the easiest way is to go and get the file back from a commit before you made any changes. You can do this as follows.

Use git log to have a look at the commit history of your branch.

Copy the hash (the long number/letter string) that identifies the commit you want.

Checkout the file that you want to reset from that commit. In this case, assuming your in the head directory (~/improver) it would be this:

git checkout 347a8a437c1284e04d01253828cac48ce11487e4 -- improver/utilities/cube_manipulation.py

This works fine if you've got a nice linear commit history, or even if you've merged in master, as long as no one else is editing the file you're interested in.

Thanks Ben, that is useful to know for future! I have made the final change.

bayliffe

🎉 Thanks for all the work on this, I think we have made the setting of vertical levels in our test data much more explicit now.

lambert-p force-pushed the mobt_693_vertical_levels branch from 0ff1f90 to d7d8142 Compare December 17, 2024 14:52

lambert-p requested a review from maxwhitemet December 17, 2024 15:22

maxwhitemet reviewed Dec 18, 2024

View reviewed changes

Kat-90 requested changes Jan 2, 2025

View reviewed changes

lambert-p mentioned this pull request Jan 3, 2025

Specifying vertical levels rather than height levels metoppv/improver_test_data#69

Closed

Kat-90 requested changes Jan 8, 2025

View reviewed changes

Kat-90 requested changes Jan 9, 2025

View reviewed changes

improver/synthetic_data/set_up_test_cubes.py Show resolved Hide resolved

Kat-90 previously approved these changes Jan 13, 2025

View reviewed changes

bayliffe requested changes Jan 14, 2025

View reviewed changes

lambert-p added 12 commits January 31, 2025 12:10

specifying vertical levels rather than height levels

bffd086

all files containing height_levels edited

b8abebf

changes made after first review

d52a1f6

updated check sums to match new kgo names

df4af06

adding a height flag

3947b2c

removing a line accidentlly added

d33bc13

working out errors

5061c3c

Fixed failing tests

2b40a37

second review changes

0226d03

height_coord changed to diagnostic_coord

b8cfe80

review changes

6ec5ebf

test_utilities change

01296aa

lambert-p dismissed Kat-90’s stale review via 01296aa January 31, 2025 13:35

lambert-p force-pushed the mobt_693_vertical_levels branch from 465514b to 01296aa Compare January 31, 2025 13:35

pre-commit tests completed

0af70a3

bayliffe requested changes Feb 5, 2025

View reviewed changes

changes and tests added

03430a0

bayliffe requested changes Feb 12, 2025

View reviewed changes

final change

c2f5ee0

bayliffe approved these changes Feb 12, 2025

View reviewed changes

bayliffe merged commit c4b7a5f into metoppv:master Feb 12, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specifying vertical levels rather than height levels #2063

Specifying vertical levels rather than height levels #2063

lambert-p commented Dec 9, 2024 •

edited by Kat-90

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

maxwhitemet left a comment

lambert-p commented Dec 19, 2024

Kat-90 commented Dec 31, 2024

Kat-90 left a comment •

edited

Loading

Kat-90 Dec 19, 2024 •

edited

Loading

Kat-90 Jan 8, 2025

bayliffe Jan 14, 2025 •

edited

Loading

Kat-90 Dec 31, 2024

lambert-p commented Jan 3, 2025

Kat-90 left a comment

Kat-90 Jan 8, 2025

Kat-90 Jan 8, 2025

Kat-90 Jan 8, 2025

Kat-90 Jan 8, 2025

Kat-90 left a comment

Kat-90 left a comment

bayliffe left a comment

bayliffe left a comment

lambert-p commented Feb 11, 2025

bayliffe left a comment

bayliffe Feb 12, 2025

lambert-p Feb 12, 2025

bayliffe left a comment

	there are two heights with the maximum height velocity.) Otherwise the highest
	there are two heights with the maximum vertical velocity.) Otherwise the highest

Specifying vertical levels rather than height levels #2063

Specifying vertical levels rather than height levels #2063

Conversation

lambert-p commented Dec 9, 2024 • edited by Kat-90 Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

maxwhitemet left a comment

Choose a reason for hiding this comment

lambert-p commented Dec 19, 2024

Kat-90 commented Dec 31, 2024

Kat-90 left a comment • edited Loading

Choose a reason for hiding this comment

Kat-90 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayliffe Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambert-p commented Jan 3, 2025

Kat-90 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kat-90 left a comment

Choose a reason for hiding this comment

Kat-90 left a comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

lambert-p commented Feb 11, 2025

bayliffe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

lambert-p commented Dec 9, 2024 •

edited by Kat-90

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

Kat-90 left a comment •

edited

Loading

Kat-90 Dec 19, 2024 •

edited

Loading

bayliffe Jan 14, 2025 •

edited

Loading