Units

[siligam]

addressing issue #6

Just implemented the bare logic to handle unit conversion. No API design yet.
Features:

Normalisation of non-standard notation of units in netcdf4 so pint can read them.
added unit definitions which are not available in pint library.

[mandresm]

Closes #6

[mandresm]

Hi @siligam, great PR! Just a few suggestions :)

[pgierz]

I think this is a good start but it still needs some work, in particular some of the key logic for integration into the rest of the toolchain is not yet there.

[pgierz]

It's getting there, keep up the good work :-)

I still have a design point, one major, and a few minor comments. The major missing part still has not yet been addressed, namely a function to hook into the pipeline. See the in-line comments for some minor revision recommendations in addition to the checklist below.

Design

• I am not sure about the chemistry units still. The unit is mol or gram. mmol is still mole, prefixed by milli, and pint should be able to handle prefixes. A mole of carbon is still a mole, it just happens to be of the element carbon, (just like a kilogram of flour is a kilogram, or sugar, or butter, or whatever you want to use to bake cookies with)

Major Suggestion

• Missing a key function to tie together all the logic

General Minor Suggestions

• f-strings everywhere for consistency
• Make sure all print statements are logger statements. It's up to you to think about what level they can have.

[pgierz]

Excellent work @siligam! I only have one more small thing that you should look at, just to be sure: The metadata attribute name on units should be plural (it already is), but I am not 100% sure so I'd like to double check.

You can also add a call to handle_unit_conversion to the generic pipeline. We can have a brief meeting so I can show you where to do that! Once that is in place, we can merge this into the main branch 🙂

[siligam]

Excellent work @siligam! I only have one more small thing that you should look at, just to be sure: The metadata attribute name on units should be plural (it already is), but I am not 100% sure so I'd like to double check.

I checked that units are indeed plural and I am sure I did use plural when setting the attribute.

You can also add a call to handle_unit_conversion to the generic pipeline. We can have a brief meeting so I can show you where to do that! Once that is in place, we can merge this into the main branch 🙂

Sure we can discuss this in a brief meeting.

[pgierz]

Looks good, we can merge this!