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.

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

Improving get_field of nomenclature module #32

Closed
leifdenby opened this issue Aug 21, 2020 · 3 comments
Closed

Improving get_field of nomenclature module #32

leifdenby opened this issue Aug 21, 2020 · 3 comments
Assignees

Comments

@leifdenby
Copy link
Collaborator

Just copying over your comments @LSaffin from #30 (comment)

The changes I would make are

  1. I would put the unit conversion in a separate function and module because it doesn't cover nomenclature
  2. When using get_field in other functions it would be better to use it from eurec4a_environment rather than eurec4a_environment.nomenclature
  3. Considering the first two points, it would be good to change the name of the function in nomenclature (e.g. to get_field_by_name) and have the function get_field in the top level module call that function and the function for unit conversion. Then other functions are calling this top level get_field and you can add other functionality that might not be related to nomenclature to it.

I can make these changes once your pull request is merged if they sound OK to you. Other questions I had:

  1. Did you find any important differences between cf_units and cfunits? As far as I could tell they were basically the same but cf_units was more active and better documented.

  2. Was using iris considered at all? This already does the unit checking as well as coordinate checking and better support for CF standard names (although the CF standard naming could be a limitation).

@leifdenby leifdenby changed the title Revising unit support in get_field of nomenclature module Improving get_field of nomenclature module Aug 21, 2020
@leifdenby
Copy link
Collaborator Author

I would put the unit conversion in a separate function and module because it doesn't cover nomenclature

This sounds good @LSaffin, yes good idea

2. When using `get_field` in other functions it would be better to use it from `eurec4a_environment` rather than `eurec4a_environment.nomenclature`

Ok, I don't have a strong preference on this. If you think that's better then let's do that.

3. Considering the first two points, it would be good to change the name of the function in nomenclature (e.g. to `get_field_by_name`) and have the function `get_field` in the top level module call that function and the function for unit conversion. Then other functions are calling this top level `get_field` and you can add other functionality that might not be related to nomenclature to it.

Yes, that sounds alright. I would like to be able to keep calling get_field(ds=ds, field_name=nom.TEMPERATURE, units='K' though, so that units keeps being an argument. Is that ok with you? Just seems a bit of a hassle to be calling two functions each time.

I can make these changes once your pull request is merged if they sound OK to you. Other questions I had:

Great!

1. Did you find any important differences between cf_units and cfunits? As far as I could tell they were basically the same but cf_units was more active and better documented.

Only that cfunits failed to install unless the UDUNITS system library is installed. In some cases we might not need to convert a unit and so I thought it would be nice to only require the system library to be there when needed. But maybe this is a bad idea and we should just enforce that people have UDUNITS installed on their system. We could also use conda instead of pip requirements, pip is just a bit more light-weight. We could change to cfunits in that case.

2. Was using iris considered at all? This already does the unit checking as well as coordinate checking and better support for CF standard names (although the CF standard naming could be a limitation).

I thought iris was a bit overkill here since it is quite a big install. Maybe if we find datasets we can't support with just cf_units we make iris a requirement?

@leosaffin
Copy link
Collaborator

leosaffin commented Aug 24, 2020

That all sounds good. I'll put together a pull request with the changes for you to approve.

That sounds like a good reason to go with cf_units. I won't worry about iris either, just making sure we don't end up remaking that library.

leifdenby pushed a commit that referenced this issue Aug 31, 2020
* Moved unit conversion to separate module
* Move get_field to top level and change to get_field_by_name and get_field_by_cf_standard_name in nomenclature.py
* Allow get_field to be called directly with a CF standard name (change argument from field_name to name to reflect this generalisation)
* Added a test for unit conversion
* Use the nomenclature names for fixing the current JOANNE names. Keeping compatibility for when they are updated
* Change Exception when units are absent from dataarray to KeyError and add test to check it is raised
@leifdenby
Copy link
Collaborator Author

That sounds like a good reason to go with cf_units. I won't worry about iris either, just making sure we don't end up remaking that library.

Yes, definitely. We might want to start using it if we find we need more functionality that iris gives. I'll close this now since I've just merged #33. Thanks again for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants