[FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata #3713

stephenworsley · 2020-05-19T13:38:59Z

In line with suggestions from #3585, default units for all DimensionalMetadata are now "unknown".

…mber

stephenworsley · 2020-05-20T10:51:22Z

Two things to consider based on the tests which were erroring:

PP loading relies on the default units in certain cases. I assume it would be appropriate to edit the PP loading to keep the same PP loading behaviour as before in most cases, however it will be worthwhile checking if this is indeed appropriate in all cases.
Certain aux_factories require coordinate units to be dimensionless (i.e. "1"). Coordinates made with default units will now break aux_factory creation.

stephenworsley · 2020-05-20T12:55:32Z

Another change which will have to be considered in order to fix the tests:

What should be the default unit for coord_categorisation.add_categorised_coord()? The default is currently "1", changing the default to "unknown" is probably correct, though there might need to be some discussion as to which unit is most correct.

…nits of "1"

stephenworsley · 2020-05-21T15:04:43Z

This line creates a coordinate in the process of collapsing, I have for the moment reverted the behaviour back to giving a unit of "1", though that may not necessarily be the most appropriate unit and this will have to be decided on:

iris/lib/iris/analysis/__init__.py

Line 805 in f5feb28

coord = iris.coords.AuxCoord(point, long_name=coord_name)

It looks like this line is specific to percentile aggregation and the coordinate should contain percentages. I have therefore chosen to give a unit of "percent" here.

pp-mo · 2020-05-29T16:34:30Z

Points you raised:

PP loading relies on the default units in certain cases. I assume it would be appropriate to edit the PP loading to keep the same PP loading behaviour as before in most cases, however it will be worthwhile checking if this is indeed appropriate in all cases.
- 👍 I'm happy that your edits preserve existing behaviour.
- I checked all cases where coords are created in PP loading.
- Plus, all the tests still pass.
Certain aux_factories require coordinate units to be dimensionless (i.e. "1"). Coordinates made with default units will now break aux_factory creation.
- I think that's ok and will be dealt with in file format loaders.
- I think we should perform a code scan of all those, but
- for now all existing tests are passing + that's good enough to start with.
What should be the default unit for coord_categorisation.add_categorised_coord()? The default is currently "1", changing the default to "unknown" is probably correct, though there might need to be some discussion as to which unit is most correct.
- I note you haven't had to change anything in iris.coord_categorisation yet,
- and again all existing tests pass.
- The individual special functions cases may want some review. Let's do that after.
This line creates a coordinate in the process of collapsing... It looks like ... the coordinate should contain percentages. I have therefore chosen to give a unit of "percent" here.
- 👍 I agree this looks right in this case.
- it only affects percentile + weighted-percentile, and I think that is ok.

pp-mo

That is literally all I can find wrong with this -- otherwise, I agree with all the choices + it already covers everything I checked for.

pp-mo · 2020-05-29T15:20:30Z

lib/iris/tests/stock/__init__.py

@@ -834,8 +834,8 @@ def jan_offset(day, year):
        units="days since 1970-01-01 00:00:00-00",
        climatological=True,
    )
-    lon_dim = DimCoord(lon, standard_name="longitude")
-    lat_dim = DimCoord(lat, standard_name="latitude")
+    lon_dim = DimCoord(lon, standard_name="longitude", units="1")


These 2 should surely be 'degrees'.

lib/iris/tests/results/netcdf/netcdf_save_no_name.cdl

pp-mo · 2020-05-29T16:52:19Z

One tiny point, and I think we can merge this.

Then we can consider knockons from this into your other outstanding PRs

[FB] [PI-3585] Change default loading unit from "1" to "unknown" (correct branch) #3709 make netcdf load unspecified units as "unknown"
[FB] [PI-3394] Unify saving behaviour of "unknown" and "no_unit" #3711 make netcdf save units of "unknown" and "no_unit" the same
[FB] [PI-3474] (WIP) Ensure flags load/save without units #3613 (or (WIP) Alternative method for assigning flags "no_unit" #3712) netcdf save/load special handling for cf flags

and the deferred issues I created above

Re-check all file format loaders for derived coordinate units #3721 re-check all file loaders for coord units
Check sensible units for categorisation functions. #3722 check all coord categorisation result units

pp-mo · 2020-06-01T11:27:12Z

Thanks @stephenworsley all checks, I'm good with this now !

… default_units_patch * 'default_units' of https://github.com/SciTools/iris: Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711) Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709) Change default units to "unknown" for all DimensionalMetadata (SciTools#3713) Update docs CubeList.extract method (SciTools#3694) Correct and improve dev-guide section on fixing graphics-tests. (SciTools#3683) New image hashes for mpl 3x2 (SciTools#3682) Switched use of datetime.weekday() to datetime.dayofwk. (SciTools#3687) Remove TestGribMessage (SciTools#3672) Removed iris.tests.integration.test_grib_load and related CML files. (SciTools#3670) Removed grib-specific test to iris-grib. (SciTools#3671) Fixed asv project name to 'scitools-iris'. (SciTools#3660) Remove cube iter (SciTools#3656) Remove test_grib_save.py (SciTools#3669) Remove test_grib2 integration tests (SciTools#3664) Remove uri callback test which is moved to iris-grib (SciTools#3665) 2v4 mergeback picks (SciTools#3668) Remove test_grib_save_rules.py which has been moved to iris-grib (SciTools#3666) Removed ununused skipIf. (SciTools#3632) Remove grib-specific test. (SciTools#3663) Remove obsolete test. (SciTools#3662)

Squashed commit of the following: commit 912f500 Author: stephenworsley <[email protected]> Date: Mon Jun 1 17:52:42 2020 +0100 Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711) commit 3171b95 Author: stephenworsley <[email protected]> Date: Mon Jun 1 16:28:45 2020 +0100 Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709) commit 3fdccac Author: stephenworsley <[email protected]> Date: Mon Jun 1 12:27:23 2020 +0100 Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)

…its_merge * upstream/default_units: fix test (SciTools#3732) Unify saving behaviour of "unknown" and "no_unit" (SciTools#3711) Change default loading unit from "1" to "unknown" (correct branch) (SciTools#3709) Change default units to "unknown" for all DimensionalMetadata (SciTools#3713)

* Change default units to "unknown" for all DimensionalMetadata (#3713) * Change default loading unit from "1" to "unknown" (correct branch) (#3709) * Unify saving behaviour of "unknown" and "no_unit" (#3711) * fix test (#3732) Co-authored-by: stephenworsley <[email protected]>

stephenworsley added 2 commits May 19, 2020 14:31

change all default units to "unknown"

a523d7a

modify pp loading to revert units to "1" for sigma and model level nu…

ed269c6

…mber

fix tests to avoid errors

5f3c5a0

stephenworsley added 6 commits May 20, 2020 14:47

fix more tests

f782c2d

fix more tests

b969838

fix more tests

b93527f

blacken

a54301c

fix more tests

b616651

fix more tests, revert collapsed behaviour to give coordinates with u…

2925b85

…nits of "1"

stephenworsley added 6 commits May 21, 2020 17:10

fix more tests

23935ca

fix more tests

892263c

fix more tests

e3905ad

blacken

0316a00

change units in percentile aggregation to "percent"

4028780

fix tests

767c7d2

stephenworsley changed the title ~~(WIP) Change default units to "unknown" for all DimensionalMetadata~~ Change default units to "unknown" for all DimensionalMetadata May 22, 2020

This was referenced May 29, 2020

Re-check all file format loaders for derived coordinate units #3721

Closed

Check sensible units for categorisation functions. #3722

Closed

pp-mo reviewed May 29, 2020

View reviewed changes

stephenworsley added 2 commits June 1, 2020 11:09

address review comments

4ead626

fix tests

e0dca25

pp-mo merged commit 3fdccac into SciTools:default_units Jun 1, 2020

pp-mo added this to the v3.0.0 milestone Jun 3, 2020

abooton changed the title ~~Change default units to "unknown" for all DimensionalMetadata~~ PI-3585: Change default units to "unknown" for all DimensionalMetadata Jun 3, 2020

abooton changed the title ~~PI-3585: Change default units to "unknown" for all DimensionalMetadata~~ [FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata #3713

[FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata #3713

stephenworsley commented May 19, 2020

stephenworsley commented May 20, 2020 •

edited

Loading

stephenworsley commented May 20, 2020

stephenworsley commented May 21, 2020 •

edited

Loading

pp-mo commented May 29, 2020 •

edited

Loading

pp-mo left a comment

pp-mo May 29, 2020

pp-mo commented May 29, 2020 •

edited

Loading

pp-mo commented Jun 1, 2020

[FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata #3713

[FB] [PI-3585] Change default units to "unknown" for all DimensionalMetadata #3713

Conversation

stephenworsley commented May 19, 2020

stephenworsley commented May 20, 2020 • edited Loading

stephenworsley commented May 20, 2020

stephenworsley commented May 21, 2020 • edited Loading

pp-mo commented May 29, 2020 • edited Loading

pp-mo left a comment

Choose a reason for hiding this comment

pp-mo May 29, 2020

Choose a reason for hiding this comment

pp-mo commented May 29, 2020 • edited Loading

pp-mo commented Jun 1, 2020

stephenworsley commented May 20, 2020 •

edited

Loading

stephenworsley commented May 21, 2020 •

edited

Loading

pp-mo commented May 29, 2020 •

edited

Loading

pp-mo commented May 29, 2020 •

edited

Loading