Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FB] [PI-3394] Unify saving behaviour of "unknown" and "no_unit" #3711

Merged
merged 3 commits into from
Jun 1, 2020
Merged

[FB] [PI-3394] Unify saving behaviour of "unknown" and "no_unit" #3711

merged 3 commits into from
Jun 1, 2020

Conversation

stephenworsley
Copy link
Contributor

Adresses #3394.

@pp-mo pp-mo mentioned this pull request May 29, 2020
Merged
@pp-mo
Copy link
Member

pp-mo commented May 29, 2020 •
edited
Loading

I think it might be a good idea to replace
unit not in ("no_unit", "unknown")
here, with
not unit.is_udunits().

That does seem to have the same effect, but I'm still not totally sure that is a correct equivalent meaning, according to what we want and what cf-units means by it.
I'll report back after looking into the implementation within cf-units...

@pp-mo
Copy link
Member

pp-mo commented Jun 1, 2020 •
edited
Loading

replace unit not in ("no_unit", "unknown") with not unit.is_udunits()

After some reading up, I conclude that I think this is a good idea.

The cf_units docs for the 'category' property explain that it currently comprises types "udunints, no-units and unknown" : https://scitools.org.uk/cf-units/docs/latest/unit.html#cf_units.Unit.category

From reading the code,

unit.isudunits() 
  === unit.category = _CATEGORY_UDUNIT
  === unit.ut_unit is not _ud.NULL_UNIT

It also has specific category methods is_unknown() and is_no_unit()

So, I think that makes sense.

However I must admit that the case ins undermined by some confusion about what is really public API. E.G. cf_units does not publish the category values, _CATEGORY_UDUNITS etc, despite the Unit.category being pubic property. etc.
I'm guessing that is just some kind of slip. Likewise, why don't they publish '_UNKNOWN_UNIT_STRING' and the like ...

@pp-mo pp-mo merged commit 912f500 into SciTools:default_units Jun 1, 2020
@pp-mo
Copy link
Member

pp-mo commented Jun 1, 2020

Thanks for humouring me !
The more I think, the more this makes sense to me : The objects' ".units" are cf_unit objects, but not all cf_units are valid udunits : so, they only get saved if they are.

@pp-mo pp-mo added this to the v3.0.0 milestone Jun 3, 2020
@abooton abooton changed the title Unify saving behaviour of "unknown" and "no_unit" Pi-3394: Unify saving behaviour of "unknown" and "no_unit" Jun 3, 2020
@abooton abooton changed the title Pi-3394: Unify saving behaviour of "unknown" and "no_unit" PI-3394: Unify saving behaviour of "unknown" and "no_unit" Jun 3, 2020
stephenworsley added a commit to stephenworsley/iris that referenced this pull request Jun 8, 2020
@stephenworsley
Merge branch 'default_units' of https://github.com/SciTools/iris into…
4f267ad 
… 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)
abooton added a commit to abooton/iris that referenced this pull request Jun 10, 2020
@abooton
Updating default_units to base a07800c
6c3832e 
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)
abooton added a commit to abooton/iris that referenced this pull request Jun 10, 2020
@abooton
Updating default_units to base a07800c
6a924b7 
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)
@abooton abooton changed the title PI-3394: Unify saving behaviour of "unknown" and "no_unit" [FB] [PI-3394] Unify saving behaviour of "unknown" and "no_unit" Jun 10, 2020
abooton added a commit to abooton/iris that referenced this pull request Aug 19, 2020
@abooton
Merge remote-tracking branch 'upstream/default_units' into default_un…
c3a3163 
…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)
trexfeathers pushed a commit that referenced this pull request Aug 19, 2020
@abooton @stephenworsley
Update default_units feature branch base1873ce14 (#3792)
e99f960 
* 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 a commit that referenced this pull request Aug 20, 2020
@abooton @stephenworsley
Merge "default_units" feature branch (#3795)
42a1964 
* 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]>
This was referenced Aug 21, 2020
Closed
Closed
@pp-mo pp-mo mentioned this pull request Jul 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@stephenworsley @pp-mo