Skip to content

Impro-1121: Rename probability cubes - 1#813

Merged
bayliffe merged 16 commits intometoppv:masterfrom
markysparks:IMPRO-1121_rename_probability_cubes
Mar 1, 2019
Merged

Impro-1121: Rename probability cubes - 1#813
bayliffe merged 16 commits intometoppv:masterfrom
markysparks:IMPRO-1121_rename_probability_cubes

Conversation

@markysparks
Copy link
Contributor

Addresses #GitHubissuenum

Rename all IMPROVER cubes that contain probabilities like probability_of_rainfall_rate to follow a specified convention that includes whether the probability is above or below a threshold within the cube name, so that the cube name better describes the contents of the cube. For example:

  • probability_of_rainfall_rate_above_threshold
  • probability_of_visibility_in_air_below_threshold

Testing:

  • Ran tests and they passed OK
  • Unit and CLI tests updated as required.

This initial PR only includes changes to the following areas:

- wxcodes (the greatest number of changes)
- threshold
- set_up_test_cubes
- cube_metadata
- weighted_blend
- nowcast lightning

A subsequent PR will be used for changes to the following:

- ensemble_calibration
- GenerateProbabilitiesFromPercentiles2D
- probability_of_snow_falling_level (special case)
- Removing the source_realizations attribute

@markysparks markysparks changed the title Impro-1121: Rename probability cubes Impro-1121: Rename probability cubes - 1 Feb 25, 2019
Copy link
Contributor

@bayliffe bayliffe left a comment

Choose a reason for hiding this comment

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

This is a lot of changes Mark, well done for being consistent throughout. A couple of name changes required throughout.

The GeneratePercentilesFromProbabilities plugin within ensemble_copula_coupling/ensemble_copula_coupling.py requires a further change that is not in your list of outstanding work. Within this plugin a percentile cube is created from a probabilities cube. This is done by ditching the probability_of prefix and removing the relative_to_threshold attribute. With the name changes implemented here we will currently get diagnostic names for these percentile cubes of the form air_temperature_above_threshold, whereas we want air_temperature. As such some changes are needed to remove these suffixes if present, essentially something like the following would do the trick:

<name>.replace('_above_threshold', '').replace('_below_threshold', '')

You should do this as a separate PR as it may (or may not) have a few knock on changes.

@metoppv metoppv deleted a comment Feb 26, 2019
@gavinevans
Copy link
Contributor

Just to highlight, that there will likely be conflicts between this PR and #814.

@markysparks markysparks requested a review from bayliffe February 27, 2019 10:54
@bayliffe
Copy link
Contributor

Only one small comment. Otherwise I am happy with the changes. Good work Mark.

@markysparks markysparks requested a review from bayliffe February 27, 2019 11:06
@fionaRust fionaRust self-requested a review February 27, 2019 11:34
bayliffe
bayliffe previously approved these changes Feb 27, 2019
Copy link
Contributor

@fionaRust fionaRust left a comment

Choose a reason for hiding this comment

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

I'm still doing some checking, but here's where I've got to so far

Sorry I didn't read the info at the top of the PR.
Other things that could change (not sure they are really part of your ticket, but they might as well be tidied up):

  • ensemble calibration helper functions tests/ensemble_calibration/ensemble_calibration/helper_functions.py sets up cubes for tests with the old style name
  • tests/utilities/test_ProbabilitiesFromPercentiles2D.py has an old style name it in

probability.

1. Rename to "probability_of_lightning"
1. Rename to "probability_of_lightning_rate_above_threshold"
Copy link
Contributor

Choose a reason for hiding this comment

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

@MoseleyS Will this cause any problems with the old operational system (am I right the code is used in gpp?)

@markysparks markysparks requested a review from bayliffe February 27, 2019 16:50
@metoppv metoppv deleted a comment Feb 27, 2019
@metoppv metoppv deleted a comment Feb 27, 2019
Copy link
Contributor

@bayliffe bayliffe left a comment

Choose a reason for hiding this comment

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

Final changes looks fine to me.

@fionaRust
Copy link
Contributor

This is ready but waiting for all PRs then merging all together.

@bayliffe bayliffe merged commit f663de5 into metoppv:master Mar 1, 2019
bayliffe added a commit that referenced this pull request Mar 1, 2019
bayliffe added a commit that referenced this pull request Mar 1, 2019
gavinevans added a commit to gavinevans/improver that referenced this pull request Mar 1, 2019
…tatypes

* upstream/master: (30 commits)
  Revert "Impro-1121: Rename probability cubes - 1 (metoppv#813)" (metoppv#818)
  Impro-1121: Rename probability cubes - 1 (metoppv#813)
  Fix pylint failure (metoppv#816)
  Extended ECC bounds for accumulations. (metoppv#811)
  Fixed iris_time_to_datetime so it doesn't modify incoming cube coordi… (metoppv#812)
  Impro 1036 time lagging fix (metoppv#807)
  IMPRO-1082: Global domain checking to allow circular coordinates (metoppv#803)
  Minor edits to support blending three or more models (metoppv#801)
  Impro1059 ecc thresholds (metoppv#802)
  Added bounds to percentile extraction dictionary for snow accumulations. (metoppv#798)
  Modified how the new weights array is created to avoid carrying forwa… (metoppv#796)
  Removing functionality we don't currently use in ChooseWeightsLinear (metoppv#799)
  Correction to allow percentiles to be calculated over the time coordinate, if there is also a forecast_reference_time coordinate on the cube. Unit tests have been amended to use the set_up_variable_cube helper function. (metoppv#785)
  Test Codecov integration (metoppv#790)
  Allow sphinx-build to be supplied manually (metoppv#797)
  IMPRO-1065: RegridLandSea made to work with multi-realization cubes. (metoppv#795)
  Update names of precipitation accumulation variables used for ECC bounds. (metoppv#783)
  Fix some pep8 issues (metoppv#794)
  Update CubeCombiner unit tests to use integer seconds and fix bug (metoppv#791)
  Making clear that the grid_metadata_identifier can be set to None to avoid making any metadata comparisons. (metoppv#793)
  ...
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
* initial changes for probability_of.... names in wxcodes and tests

* further changes to wxcodes stuff, threshold.py and unit tests for both

* separate naming of cubes now required for above/below in threshold.py

* applied naming changes to set_up_test_cubes.py and associated unit test

* formatting fixes/reverts

* formatting fixes

* fix incorrect renaming of cloud diagnostic sfc-1000ft

* correction to wxcode bats test, further minor updates

* update diagnostic probability names

* update lightning nowcast files

* fix missed update for cloud diagnostic name

* update wxcode bats tests to work with new inpute file/cube probability names

* some formatting fixes

* changes made in response to reviewers comments

* remove uneccessary np.flipud from unit test

* doc changes in response to reviewers comments
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants