Adds handling of a model-id-attr to wxcode-modal #1634

MoseleyS · 2021-12-23T10:40:28Z

Updates checksums for wxcode-modal following addition of mosg__model_configuration to wxcode-modal gridded test input data.
Adds option to include this attribute in the output data.
Uses this option in acceptance test and updates checksums for KGO.

Addresses https://github.com/metoppv/mo-blue-team/issues/92

After working on the above issue, I found that the wxcode-modal acceptance test data were missing a required attribute. The attribute has been added to the improver_tests branch mobt_92_wxcode_mode and this PR updates the checksums. However, testing in the IMPROVER suite showed that the attribute is not output by the wxcode-modal CLI, so this PR now also includes code to handle this attribute.

The real problem is that the IMPROVER suite does not pass in the model configuration attribute when generating weather symbols. (addressed by https://github.com/MetOffice/improver_suite/pull/1178)

Testing:

Ran tests and they passed OK

codecov · 2021-12-23T10:45:45Z

Codecov Report

Merging #1634 (76bc64a) into hotfix_branch_1.0.3 (5f6988c) will increase coverage by 0.43%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           hotfix_branch_1.0.3    #1634      +/-   ##
=======================================================
+ Coverage                97.63%   98.07%   +0.43%     
=======================================================
  Files                      111      110       -1     
  Lines                    10664    10018     -646     
=======================================================
- Hits                     10412     9825     -587     
+ Misses                     252      193      -59

Impacted Files	Coverage Δ
improver/wxcode/modal_code.py	`100.00% <100.00%> (ø)`
improver/regrid/bilinear.py	`98.88% <0.00%> (-0.77%)`	⬇️
improver/regrid/landsea2.py	`98.18% <0.00%> (-0.74%)`	⬇️
...semble_copula_coupling/ensemble_copula_coupling.py	`99.06% <0.00%> (-0.53%)`	⬇️
...ometric_calculations/psychrometric_calculations.py	`99.21% <0.00%> (-0.01%)`	⬇️
improver/regrid/grid.py	`100.00% <0.00%> (ø)`
improver/metadata/amend.py	`100.00% <0.00%> (ø)`
improver/ensemble_copula_coupling/utilities.py	`100.00% <0.00%> (ø)`
...prover/ensemble_copula_coupling/numba_utilities.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f6988c...76bc64a. Read the comment docs.

gavinevans

Thanks @MoseleyS 👍

I've added a couple of comments relating to updating the title attribute in the acceptance test data but the bulk of this PR seems fine.

improver_tests/acceptance/SHA256SUMS

gavinevans

Thanks for the clarification of the preferred titles for the spot and gridded cubes. These titles now seem correct based on the agreed preferred format. However, I've noticed that some units now seem to have changed in the acceptance test data with more details in my comment below.

gavinevans · 2022-01-07T09:05:13Z

improver_tests/acceptance/SHA256SUMS

-a6698b2aae8bc223a2d362ccb3047dd9882e6c0df21962e55c577e1ad5850e29  ./wxcode-modal/spot_ties/20201209T1700Z-weather_symbols-PT01H.nc
-9290b2899eedfca038226c6707e100025ce68b0798972a60360845ccf36858d2  ./wxcode-modal/spot_ties/20201209T1800Z-weather_symbols-PT01H.nc
-ceb7106e5b8919590327a75d4a8b740fb1a2c7cb5a271a5bf980f0c1396c7d22  ./wxcode-modal/spot_ties/kgo.nc
+f65618c59e938286feee2720bb3be6bf6b1d32b1e439319f9eeaa538cf605fcf  ./wxcode-modal/blend_mismatch_inputs/20201209T0700Z-weather_symbols-PT01H.nc


It looks like the units for the wmo_id and met_office_site_id coordinates have changed in this acceptance test branch from no_unit to unknown. I think no_unit is preferred for these coordinates e.,g. in build_spotdata_cube, so could this be corrected?

Took me a while to work out that I'd used the Iris load and save functions rather than the IMPROVER versions. I believe that ONLY the metadata I intended to change have now changed.

gavinevans

Thanks @MoseleyS 👍

Unfortunately, I noticed that a few of the files still have units for the wmo_id and met_office_site_id coordinates, which aren't what we would expect. It might be worth checking as it seems like predominately the known good outputs are affected where the units have changed but still aren't correct.

gavinevans · 2022-01-07T14:20:09Z

improver_tests/acceptance/SHA256SUMS

+4655358c22abdd0acfd5f4e08f156a7f4f629a1764dd8314f28f121964725078  ./wxcode-modal/spot_input/20201209T1600Z-weather_symbols-PT01H.nc
+28ee3f9d419bb9766cc68e1c74fa9e8e5010dc63a600635a54356a33795f6644  ./wxcode-modal/spot_input/20201209T1700Z-weather_symbols-PT01H.nc
+073fd0175e35afcf7f9fe61fedfa63bbd9ea5299f68d8b0287bc30085339b942  ./wxcode-modal/spot_input/20201209T1800Z-weather_symbols-PT01H.nc
+2041f9ed3d1c350518dab5d537652165e58bbe92a003f5c2dfaa0ff94ccce1e1  ./wxcode-modal/spot_input/kgo.nc


In this file, it seems like the units for the wmo_id and met_office_site_id has changed from unknown to 1. I think that the units should actually be no_unit.

gavinevans · 2022-01-07T14:25:45Z

improver_tests/acceptance/SHA256SUMS

+6fe4b1563f0fbb4579dba46b12654f983cd6486f39986d52231aad5d18acf508  ./wxcode-modal/spot_ties/20201209T1600Z-weather_symbols-PT01H.nc
+d506afe1572ded010805662f057926ba19367a8bf0c9f700380b6b63ab440399  ./wxcode-modal/spot_ties/20201209T1700Z-weather_symbols-PT01H.nc
+1c0c3b4fcc95ea2d5c9658ebd42cbee8a2043afa607843e0e9013d273f7fc460  ./wxcode-modal/spot_ties/20201209T1800Z-weather_symbols-PT01H.nc
+8e8b8daeff5cb0f77f8bd36d1e6eee24534f7e1af42cbb8e119a6a3df175e4fc  ./wxcode-modal/spot_ties/kgo.nc


In this file, it seems like the units for the wmo_id and met_office_site_id has changed from unknown to 1. I think that the units should actually be no_unit.

gavinevans · 2022-01-07T14:26:20Z

improver_tests/acceptance/SHA256SUMS

+4655358c22abdd0acfd5f4e08f156a7f4f629a1764dd8314f28f121964725078  ./wxcode-modal/blend_mismatch_inputs/20201209T1600Z-weather_symbols-PT01H.nc
+28ee3f9d419bb9766cc68e1c74fa9e8e5010dc63a600635a54356a33795f6644  ./wxcode-modal/blend_mismatch_inputs/20201209T1700Z-weather_symbols-PT01H.nc
+073fd0175e35afcf7f9fe61fedfa63bbd9ea5299f68d8b0287bc30085339b942  ./wxcode-modal/blend_mismatch_inputs/20201209T1800Z-weather_symbols-PT01H.nc
+2041f9ed3d1c350518dab5d537652165e58bbe92a003f5c2dfaa0ff94ccce1e1  ./wxcode-modal/blend_mismatch_inputs/kgo.nc


In this file, it seems like the units for the wmo_id and met_office_site_id has changed from unknown to 1. I think that the units should actually be no_unit.

gavinevans · 2022-01-07T14:36:43Z

improver_tests/acceptance/SHA256SUMS

+9fbcfc0e0c3f6af0d07690c6be12e2f641123168af0c7e165b8e0ec6a2ca6f98  ./wxcode-modal/single_input/20201210T0000Z-weather_symbols-PT01H.nc
+4e52a42de445064a987da6f93b7e888c0a8ca974063a4d9cf9818a57d9b60367  ./wxcode-modal/single_input/kgo.nc


For both of these files, the wmo_id and met_office_site_id units are 1 in both master and the acceptance test branch but I think it should be no_unit.

I've recreated the KGO (the associated tests were failing and I hadn't noticed). These units look correct now.

gavinevans

Thanks @MoseleyS 👍

I've added a couple more comments about the wmo_id and met_office_site_id coordinate units.

gavinevans · 2022-01-10T08:20:20Z

improver_tests/acceptance/SHA256SUMS

+182211dba9b8d18c2d48dce89f2871854ae9baa9457bc9cfad16c1cea80faf09  ./wxcode-modal/gridded_ties/20201209T1700Z-weather_symbols-PT01H.nc
+2ed494728e73e8f740680ae090504770ee6b60131d543698c640db5ed4e08598  ./wxcode-modal/gridded_ties/20201209T1800Z-weather_symbols-PT01H.nc
+fab89f125fe7b016a35fcbf52cd9b713cfa28c7009b90150f289f47d7bea6b12  ./wxcode-modal/gridded_ties/kgo.nc
+9fbcfc0e0c3f6af0d07690c6be12e2f641123168af0c7e165b8e0ec6a2ca6f98  ./wxcode-modal/single_input/20201210T0000Z-weather_symbols-PT01H.nc


This input cube has wmo_id and met_office_site_id coordinates with units of 1. This should be no_units. Changing this should hopefully also correct the units in the known good output.

The units of the wmo_id and met_office_site_id are still 1, however, this isn't critical to this PR.

gavinevans · 2022-01-10T09:22:46Z

improver_tests/acceptance/SHA256SUMS

+4655358c22abdd0acfd5f4e08f156a7f4f629a1764dd8314f28f121964725078  ./wxcode-modal/blend_mismatch_inputs/20201209T1600Z-weather_symbols-PT01H.nc
+28ee3f9d419bb9766cc68e1c74fa9e8e5010dc63a600635a54356a33795f6644  ./wxcode-modal/blend_mismatch_inputs/20201209T1700Z-weather_symbols-PT01H.nc
+073fd0175e35afcf7f9fe61fedfa63bbd9ea5299f68d8b0287bc30085339b942  ./wxcode-modal/blend_mismatch_inputs/20201209T1800Z-weather_symbols-PT01H.nc
+a6b3f48f8c2003c2d84da0a4cb8634bed24f0d54594d6cc9235c470858f49c19  ./wxcode-modal/blend_mismatch_inputs/kgo.nc


I think that the known good outputs have wmo_id and met_office_site_id coordinates with unknown units, rather than no_unit as it seems like even though no_unit is in the cube prior to saving, no units are actually saved for e.g. the wmo_id coordinate (as seen from ncdump -h). Therefore loading the saved known good outputs results in the wmo_id and met_office_site_id coordinates having an unknown unit.

I'm not quite sure why the units for the wmo_id and met_office_site_id coordinates aren't being saved in kgo, given that they're present in the input cubes. Should there be any concern about this?

@bayliffe has pointed out that (possibly related to Iris 3 or other dependent modules), it isn't possible to save the cubes with no_units units, which is why unknown units are in the outputs.

benfitzpatrick · 2022-01-10T12:17:25Z

Could we redirect this to point to hotfix_branch_1.0.3, provisionally destined for new tag 1.0.4? cc @tjtg

…wxcode-modal gridded test data

- Adds a test for this - Updates acceptance tests to use this - and updates checksums as acceptance test inputs modified so that - mosg__model_configuration is set - title is consistent with a blended data-set

… load/save wrappers.

…fix 1.0.3

MoseleyS · 2022-01-18T09:50:35Z

Rebased code and acceptance test branches on hotfix branches. I'm not sure why two extra checksums have changed, I think that something was out of date somewhere. I'll double check this later, but @gavinevans can re-review this now.

benfitzpatrick · 2022-01-18T10:11:49Z

improver/wxcode/modal_code.py

+            for source_cube in cubes:
+                for model in source_cube.attributes[self.model_id_attr].split(" "):
+                    contributing_models.update([model])
+            # iris concatenates string coordinates as a "|"-separated string


Why this comment?

I've no idea. It must have meant something to me once. I've removed it.

gavinevans · 2022-01-18T10:46:18Z

improver_tests/acceptance/SHA256SUMS

@@ -464,6 +464,7 @@ b9d56c28ab78f296842865d3d6863d5017d6928e45632b00bf05f086fc651969  ./spot-extract
 b0cfd198d69d5d5f85775542c77dc4a2a0a3092d407e338d74c72618e5de7b1e  ./spot-extract/inputs/enukx_temperature_percentiles.nc
 7c1546a4a0eac805483d09a99268767ede51b5649eb545e002a1e02c9f3f43f2  ./spot-extract/inputs/enukx_temperature_realizations.nc
 f67aca830966488cb7ac76fbd91c50ccf2494898233557c3c4070244088a296c  ./spot-extract/inputs/enukx_temperature_thresholds.nc
+f84730d9815d1b488915eed704cadc311eb43135015395cb75cbd8cd4b9985cd  ./spot-extract/inputs/enukx_temperature_thresholds_multi_time.nc


I think that these two spot extract files exist here because you need to be using the acceptance test data hotfix branch. I think these files are only in master, rather than the hotfix branch.

After running several checks, I've found that the checksums for release 1.0.2 don't match. As you pointed out, release 1.0.3 did not change the acceptance tests, so therefore these changes here are the result of that error in release 1.0.2.

Hmmm... Now I'm not so sure. I think this might have crept in in one of my review fixes. I'll get there eventually...

Found the problem. My acceptance test branch rebase was incorrectly still including a commit on master (the one incorrectly labelled 1.0.2). I've now straightened it out and force-pushed it, so there's only one branch again now (sorry @bayliffe). The checksums now show that only the targetted files have been changed by this PR. (phew!)

gavinevans · 2022-01-18T11:05:33Z

improver_tests/acceptance/SHA256SUMS

+182211dba9b8d18c2d48dce89f2871854ae9baa9457bc9cfad16c1cea80faf09  ./wxcode-modal/gridded_ties/20201209T1700Z-weather_symbols-PT01H.nc
+2ed494728e73e8f740680ae090504770ee6b60131d543698c640db5ed4e08598  ./wxcode-modal/gridded_ties/20201209T1800Z-weather_symbols-PT01H.nc
+fab89f125fe7b016a35fcbf52cd9b713cfa28c7009b90150f289f47d7bea6b12  ./wxcode-modal/gridded_ties/kgo.nc
+9fbcfc0e0c3f6af0d07690c6be12e2f641123168af0c7e165b8e0ec6a2ca6f98  ./wxcode-modal/single_input/20201210T0000Z-weather_symbols-PT01H.nc


The units of the wmo_id and met_office_site_id are still 1, however, this isn't critical to this PR.

gavinevans

Thanks, @MoseleyS

I think that this is fine apart from the couple of additional acceptance test files.

bayliffe

I'm happy with this, and the acceptance test data now seem fine; improved in fact (thanks for correcting the precipitation units (mm --> m) in the wxcode basic inputs).

MoseleyS · 2022-01-18T15:26:59Z

Acceptance test data are now in hotfix_branch_1.0.4 and tagged as 1.0.4.

* master: remove cycle (metoppv#1657) Minor edits to remove raising unnecessary warnings. (metoppv#1646) Adds handling of a model-id-attr to wxcode-modal (metoppv#1634) # Conflicts: # improver_tests/acceptance/SHA256SUMS

* master: Modifies wxcode check_tree utility function to report issues with unreachable nodes (#1637) remove cycle (#1657) Minor edits to remove raising unnecessary warnings. (#1646) Change pandas DataFrame.at to DataFrame.loc (#1655) Adds handling of a model-id-attr to wxcode-modal (#1634)

* master: Modifies wxcode check_tree utility function to report issues with unreachable nodes (metoppv#1637) remove cycle (metoppv#1657) Minor edits to remove raising unnecessary warnings. (metoppv#1646) Change pandas DataFrame.at to DataFrame.loc (metoppv#1655) Adds handling of a model-id-attr to wxcode-modal (metoppv#1634) # Conflicts: # improver_tests/acceptance/SHA256SUMS

…factor_tidy_CLIs * feature_branch_nbhood_refactor: Mobt 157 nbhood refactor consolidate unit tests rebased (metoppv#1664) Mobt 157 nbhood refactor consolidate unit tests part1 (metoppv#1665) Adds a filter to the combine CLI for mismatching realizations (metoppv#1656) Reduce the memory requirements for read-the-docs (metoppv#1672) Further doc-building fixes. (metoppv#1671) DOC Fix intersphinx links for docs (metoppv#1668) Mobt 157 nbhood refactor sort out base classes (metoppv#1653) Modifies wxcode check_tree utility function to report issues with unreachable nodes (metoppv#1637) remove cycle (metoppv#1657) Minor edits to remove raising unnecessary warnings. (metoppv#1646) Change pandas DataFrame.at to DataFrame.loc (metoppv#1655) MOBT-154: Reunification of wx decision trees (metoppv#1639) Consolidate scale parameter usage across EMOS and ECC (metoppv#1642) Adds handling of a model-id-attr to wxcode-modal (metoppv#1634)

* Updates checksums following addition of mosg__model_configuration to wxcode-modal gridded test data * Adds model-id-attr to wxcode modal - Adds a test for this - Updates acceptance tests to use this - and updates checksums as acceptance test inputs modified so that - mosg__model_configuration is set - title is consistent with a blended data-set * Black and flake8 * Debugs new tests to that the new parametrized variable is available where it's needed * Updates checksums with gridded wxcode-modal attribute update * Updates checksums with spot wxcode-modal attribute update * Updates checksums after merging with master * Updates checksums following reapplication of meta-data using IMPROVER load/save wrappers. * Updates checksums following recreation of KGO. * Recreates checksums following rebase of acceptance test branch to hotfix 1.0.3 * Removes useless comment * Corrects checksums following rebase of acceptance test data branch

MoseleyS self-assigned this Dec 23, 2021

MoseleyS assigned MoseleyS and unassigned MoseleyS Dec 23, 2021

MoseleyS changed the title ~~Updates checksums for wxcode-modal following addition of mosg__model_configuration to …~~ Adds handling of a model-id-attr to wxcode-modal Jan 4, 2022

MoseleyS removed their assignment Jan 4, 2022

gavinevans self-assigned this Jan 6, 2022

gavinevans requested changes Jan 6, 2022

View reviewed changes

improver_tests/acceptance/SHA256SUMS Outdated Show resolved Hide resolved

improver_tests/acceptance/SHA256SUMS Outdated Show resolved Hide resolved

gavinevans assigned MoseleyS and unassigned gavinevans Jan 6, 2022

gavinevans mentioned this pull request Jan 6, 2022

Fix metadata in apply-emos-coefficients/sites/additional_predictor files #1596

Closed

MoseleyS assigned gavinevans and unassigned MoseleyS Jan 7, 2022

gavinevans requested changes Jan 7, 2022

View reviewed changes

gavinevans assigned MoseleyS and unassigned gavinevans Jan 7, 2022

MoseleyS assigned gavinevans and unassigned MoseleyS Jan 7, 2022

gavinevans requested changes Jan 7, 2022

View reviewed changes

gavinevans assigned MoseleyS and unassigned gavinevans Jan 7, 2022

MoseleyS assigned gavinevans and unassigned MoseleyS Jan 7, 2022

gavinevans requested changes Jan 10, 2022

View reviewed changes

gavinevans assigned MoseleyS and unassigned gavinevans Jan 10, 2022

gavinevans mentioned this pull request Jan 17, 2022

MOBT-199: mosg__model_run in metadata interpreter #1644

Merged

2 tasks

MoseleyS added 2 commits January 17, 2022 15:27

Updates checksums following addition of mosg__model_configuration to …

92a9fd1

…wxcode-modal gridded test data

Adds model-id-attr to wxcode modal

67fc8bc

- Adds a test for this - Updates acceptance tests to use this - and updates checksums as acceptance test inputs modified so that - mosg__model_configuration is set - title is consistent with a blended data-set

MoseleyS added 4 commits January 17, 2022 15:27

Updates checksums with spot wxcode-modal attribute update

db97cfd

Updates checksums after merging with master

d2f5930

Updates checksums following reapplication of meta-data using IMPROVER…

7b6a420

… load/save wrappers.

Updates checksums following recreation of KGO.

dab2d37

MoseleyS force-pushed the mobt_92_wxcode_mode branch from 85583dd to dab2d37 Compare January 17, 2022 15:27

MoseleyS changed the base branch from master to hotfix_branch_1.0.3 January 17, 2022 15:28

Recreates checksums following rebase of acceptance test branch to hot…

72f163e

…fix 1.0.3

MoseleyS assigned gavinevans and unassigned MoseleyS Jan 18, 2022

benfitzpatrick reviewed Jan 18, 2022

View reviewed changes

Removes useless comment

1f47b2b

gavinevans reviewed Jan 18, 2022

View reviewed changes

gavinevans requested changes Jan 18, 2022

View reviewed changes

bayliffe self-assigned this Jan 18, 2022

Corrects checksums following rebase of acceptance test data branch

76bc64a

bayliffe approved these changes Jan 18, 2022

View reviewed changes

bayliffe removed their assignment Jan 18, 2022

gavinevans approved these changes Jan 18, 2022

View reviewed changes

gavinevans assigned MoseleyS and unassigned gavinevans Jan 18, 2022

MoseleyS merged commit 00af0b1 into metoppv:hotfix_branch_1.0.3 Jan 18, 2022

MoseleyS deleted the mobt_92_wxcode_mode branch January 18, 2022 15:21

benfitzpatrick mentioned this pull request Jan 18, 2022

Merge back 1.0.4 #1650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds handling of a model-id-attr to wxcode-modal #1634

Adds handling of a model-id-attr to wxcode-modal #1634

MoseleyS commented Dec 23, 2021 •

edited

Loading

codecov bot commented Dec 23, 2021 •

edited

Loading

gavinevans left a comment

gavinevans left a comment

gavinevans Jan 7, 2022

MoseleyS Jan 7, 2022

gavinevans left a comment

gavinevans Jan 7, 2022

gavinevans Jan 7, 2022

gavinevans Jan 7, 2022

gavinevans Jan 7, 2022

MoseleyS Jan 7, 2022

gavinevans left a comment

gavinevans Jan 10, 2022

gavinevans Jan 18, 2022

gavinevans Jan 10, 2022

gavinevans Jan 17, 2022

benfitzpatrick commented Jan 10, 2022

MoseleyS commented Jan 18, 2022

benfitzpatrick Jan 18, 2022

MoseleyS Jan 18, 2022

gavinevans Jan 18, 2022

MoseleyS Jan 18, 2022

MoseleyS Jan 18, 2022

MoseleyS Jan 18, 2022

gavinevans Jan 18, 2022

gavinevans left a comment

bayliffe left a comment

MoseleyS commented Jan 18, 2022

		9fbcfc0e0c3f6af0d07690c6be12e2f641123168af0c7e165b8e0ec6a2ca6f98 ./wxcode-modal/single_input/20201210T0000Z-weather_symbols-PT01H.nc
		4e52a42de445064a987da6f93b7e888c0a8ca974063a4d9cf9818a57d9b60367 ./wxcode-modal/single_input/kgo.nc

Adds handling of a model-id-attr to wxcode-modal #1634

Adds handling of a model-id-attr to wxcode-modal #1634

Conversation

MoseleyS commented Dec 23, 2021 • edited Loading

codecov bot commented Dec 23, 2021 • edited Loading

Codecov Report

gavinevans left a comment

Choose a reason for hiding this comment

gavinevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinevans 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

Choose a reason for hiding this comment

gavinevans 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

benfitzpatrick commented Jan 10, 2022

MoseleyS commented Jan 18, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavinevans left a comment

Choose a reason for hiding this comment

bayliffe left a comment

Choose a reason for hiding this comment

MoseleyS commented Jan 18, 2022

MoseleyS commented Dec 23, 2021 •

edited

Loading

codecov bot commented Dec 23, 2021 •

edited

Loading