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

custom unit formatter #278

Merged
merged 15 commits into from
Jan 5, 2022
Merged

custom unit formatter #278

merged 15 commits into from
Jan 5, 2022

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Jan 3, 2022

The combination of pint's new-ish custom formatter feature and xclim's unit formatting function (see Ouranosinc/xclim#780). Only the short formatter for now because pint does not pass the unit modifier to the formatter (i.e. to support both short and long version, we need a change to pint first).

  • Tests added
  • fully documented
  • changelog updated
  • licensing: how do you want to handle this? At the moment there's a module docstring declaring everything in the module to be vendored from metpy, but that's not true anymore.

cc @aulemahal

@keewis keewis changed the title unit formatter custom unit formatter Jan 3, 2022
@aulemahal
Copy link
Contributor

Nice! If your last point was about "sublicensing" xclim, I don't think it's really needed. I mean, as a core dev of xclim, I am totally ok with cf-xarray simply copying the code. To be certain we can ask @huard, who I think wrote this function (pint2cfunits).

@keewis
Copy link
Contributor Author

keewis commented Jan 4, 2022

that was directed at @dcherian: I only have experience with vendoring code (which is including the code without asking for permission) so I wasn't sure what to do in this case.

A simple comment stating that "this was moved here from xclim", or modifying the notice about metpy to not cover everything in the module might probably suffice?

@dcherian
Copy link
Contributor

dcherian commented Jan 4, 2022

Not sure what the right answer is.

Practically, I think modifying the notice at the top to specifically mention this function should be OK? We can also add a. comment above this function to make it extra clear.

It does seem like we need to include the text of the BSD-3-clause license somewhere

@keewis
Copy link
Contributor Author

keewis commented Jan 4, 2022

not sure if that's the best way, but I think if the xclim developers (in this case @huard?) tell us it's fine without that we would be relicensing it to cf-xarray's license?

@dcherian
Copy link
Contributor

dcherian commented Jan 4, 2022

xclim and cf_xarray are both apache-2 and apache-2 allows redistributing as long. as the new. license is compatible with apache-2

The super safe thing would be to create a new formatting.py and import it here.

@keewis
Copy link
Contributor Author

keewis commented Jan 4, 2022

The super safe thing would be to create a new formatting.py and import it here.

or we reorganize the current one to not have the notice at the top, but divide it into sections containing code from the same source. For now, this is easy: there's no dependency between the unit parsing and the unit formatting code.

@dcherian
Copy link
Contributor

dcherian commented Jan 4, 2022

OK sounds good to me.

@keewis
Copy link
Contributor Author

keewis commented Jan 5, 2022

not sure what the issue with RTD is, but in any case this should be ready for another round of reviews.

Edit: still not sure about the license, let's see what @huard and @aulemahal think

@dcherian
Copy link
Contributor

dcherian commented Jan 5, 2022

LGTM.

Is pint likely to support passing modifiers soon? Should we instead register cfshort and cflong?

@keewis
Copy link
Contributor Author

keewis commented Jan 5, 2022

that depends on whether or not I can send in a PR... I think that should not be too much effort, but I won't really know until I work on it

@dcherian dcherian merged commit 44ef351 into xarray-contrib:main Jan 5, 2022
@dcherian
Copy link
Contributor

dcherian commented Jan 5, 2022

Thanks @keewis. This is a great contribution

@keewis keewis deleted the formatter branch January 5, 2022 20:18
@huard
Copy link

huard commented Jan 10, 2022

For the record, I'm happy to see this go into cf-xarray.

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

Successfully merging this pull request may close these issues.

4 participants