clean-up and implement the conversion methods #11

keewis · 2020-06-27T12:39:43Z

As described in #6, this aims to implement working Dataset.pint.to and DataArray.pint.to methods.

keewis · 2020-06-28T13:05:35Z

@jthielen, @TomNicholas: here's an overview of the current implementation of to:

usage examples of DataArray.pint.to

In [1]: import xarray as xr 
   ...: import numpy as np 
   ...: import pintxarray 
   ...: import pint 
   ...: ureg = pint.UnitRegistry()

In [2]: da = xr.DataArray( 
   ...:     dims="x", 
   ...:     data=np.linspace(0, 1, 4) * ureg.m, 
   ...:     coords={"u": ("x", [2, 4, 6, 8] * ureg.s)}, 
   ...: )

In [3]: da.pint.to("mm")
Out[3]: 
<xarray.DataArray (x: 4)>
<Quantity([   0.          333.33333333  666.66666667 1000.        ], 'millimeter')>
Coordinates:
    u        (x) int64 <Quantity([2 4 6 8], 'second')>
Dimensions without coordinates: x

In [4]: da.pint.to(ureg.mm)
Out[4]: 
<xarray.DataArray (x: 4)>
<Quantity([   0.          333.33333333  666.66666667 1000.        ], 'millimeter')>
Coordinates:
    u        (x) int64 <Quantity([2 4 6 8], 'second')>
Dimensions without coordinates: x

In [5]: da.pint.to({da.name: "mm"})
Out[5]: 
<xarray.DataArray (x: 4)>
<Quantity([   0.          333.33333333  666.66666667 1000.        ], 'millimeter')>
Coordinates:
    u        (x) int64 <Quantity([2 4 6 8], 'second')>
Dimensions without coordinates: x

In [6]: da.pint.to("mm", u="ms")
Out[6]: 
<xarray.DataArray (x: 4)>
<Quantity([   0.          333.33333333  666.66666667 1000.        ], 'millimeter')>
Coordinates:
    u        (x) float64 <Quantity([2000. 4000. 6000. 8000.], 'millisecond')>
Dimensions without coordinates: x

In [7]: da.pint.to({da.name: "mm", "u": "ms"})
Out[7]: 
<xarray.DataArray (x: 4)>
<Quantity([   0.          333.33333333  666.66666667 1000.        ], 'millimeter')>
Coordinates:
    u        (x) float64 <Quantity([2000. 4000. 6000. 8000.], 'millisecond')>
Dimensions without coordinates: x

In [8]: da.pint.to({da.name: "mm"}, u="ms")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-9-e092d5c447d0> in <module>
----> 1 da.pint.to({da.name: "mm"}, u="ms")

.../pintxarray/accessors.py in to(self, units, registry, **unit_kwargs)
    265             )
    266 
--> 267         units = either_dict_or_kwargs(units, unit_kwargs, "to")
    268 
    269         return conversion.convert_units(self.da, units)

.../pintxarray/accessors.py in either_dict_or_kwargs(positional, keywords, method_name)
     43             )
     44         if keywords:
---> 45             raise ValueError(
     46                 "cannot specify both keyword and positional "
     47                 f"arguments to .{method_name}"

ValueError: cannot specify both keyword and positional arguments to .to

usage examples of Dataset.pint.to

In [1]: import xarray as xr 
   ...: import numpy as np 
   ...: import pintxarray 
   ...: import pint 
   ...: ureg = pint.UnitRegistry()

In [2]: ds = xr.Dataset( 
   ...:     data_vars={"a": ("x", np.linspace(0, 1, 4) * ureg.m)}, 
   ...:     coords={"u": ("x", [2, 4, 6, 8] * ureg.ms)}, 
   ...: )

In [3]: ds.pint.to(a="mm", u="ms")
Out[3]: 
<xarray.Dataset>
Dimensions:  (x: 4)
Coordinates:
    u        (x) int64 <Quantity([2 4 6 8], 'millisecond')>
Dimensions without coordinates: x
Data variables:
    a        (x) float64 <Quantity([   0.          333.33333333  666.66666667...

In [4]: ds.pint.to({"a": "mm", "u": "ms"})
Out[4]: 
<xarray.Dataset>
Dimensions:  (x: 4)
Coordinates:
    u        (x) int64 <Quantity([2 4 6 8], 'millisecond')>
Dimensions without coordinates: x
Data variables:
    a        (x) float64 <Quantity([   0.          333.33333333  666.66666667...

In [5]: ds.pint.to({"a": "mm"}, u="ms")
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-49ddaee6c1c1> in <module>
----> 1 ds.pint.to({"a": "mm"}, u="ms")

.../pintxarray/accessors.py in to(self, units, **unit_kwargs)
    376 
    377     def to(self, units=None, **unit_kwargs):
--> 378         units = either_dict_or_kwargs(units, unit_kwargs, "to")
    379 
    380         return conversion.convert_units(self.ds, units)

.../pintxarray/accessors.py in either_dict_or_kwargs(positional, keywords, method_name)
     43             )
     44         if keywords:
---> 45             raise ValueError(
     46                 "cannot specify both keyword and positional "
     47                 f"arguments to .{method_name}"

ValueError: cannot specify both keyword and positional arguments to .to

In a future PR we could then add support for contexts via a contexts keyword-only argument (variables named contexts then cannot be converted using kwargs but this is still possible using the positional argument).

Also, we could add pseudo-support for converting units in indexes: take the string unit from the units attribute, use it to construct a quantity, convert it to the new units, set the new index values using the magnitude of the converted quantity, and then update the attribute.

keewis · 2020-06-28T13:13:46Z

Also, @TomNicholas: there's still only a single check visible. I can't seem to be able to set up an account so I can't verify this, but reading the documentation it seems that it needs read / write access to checks.

keewis · 2020-07-01T23:10:55Z

@TomNicholas, this should be ready for review and merge.

TomNicholas

This looks good to me!

One comment: there is now this separation between high-level functions (e.g. attach_units) and internal ones (e.g. array_attach_units). Should they be separated with preceding underscores? Should some of the top-level ones be public API? In particular the testing assert functions might be useful for users or downstream developers.

TomNicholas · 2020-07-08T10:58:23Z

pintxarray/tests/test_conversion.py

+            pytest.param(None, id="without registry"),
+            pytest.param(unit_registry, id="with registry"),


I did not know you could assign tests ids like this - cool!

keewis · 2020-07-08T12:13:12Z

hmm... for now I'd say let's consider everything imported by __init__.py public and everything else private. Once we have a more thorough API documentation, we could state that everything included there is public and everything else isn't (which I think is what xarray does).

I agree that adding a pintxarray.testing module to the public API would definitely be useful.

jthielen · 2020-07-08T14:57:16Z

@keewis The implementation of to here looks great. I agree that it would be good to follow-up on this with contexts support and index conversion pseudo-support. The latter is particularly important in most of my own use cases, so if you don't get around to implementing it soon, I'll try to do so. In MetPy I added that index conversion on a separate accessor method, but here it looks like it could integrate well into the main to method.

keewis · 2020-07-08T16:42:16Z

~~with two approvals I guess it is time to merge?~~ not yet, the docstrings are missing

Also, @jthielen, if you already implemented the pseudo-index support once it might be faster for you to do so again. Go ahead if you want to and have the time.

keewis · 2020-07-08T16:44:35Z

ah, with the relocation the CI setup is gone. @TomNicholas, do you think you could set this up again?

keewis · 2020-07-08T19:06:26Z

I added the docstrings (and found two bugs in the process), so this is ready for another round of reviews.

jthielen

This looks good to me pending CI getting fixed and passing. Just a couple questions (that don't need to hold up merging) on things I noticed when looking at this closer:

pint_xarray/accessors.py

jthielen · 2020-07-08T21:07:05Z

pint_xarray/accessors.py

+
+        units = either_dict_or_kwargs(units, unit_kwargs, "to")
+
+        return conversion.convert_units(self.da, units)

    def to_base_units(self):


Should to_base_units also be adjusted analogously to use a util from conversion or is it fine as-is?

to_base_units uses Quantity.to_base_units, so I'm not quite sure how we'd be able to use a conversion function here. Did you want to add a function to conversion that calls .to_base_units? If so, then yes, we could use that to reduce duplicated code.

We should also allow passing in a dict / kwargs to control which of the DataArray's variables should be converted (defaulting to "convert all variables").

Both should be a new PR, though.

jthielen · 2020-07-08T21:09:27Z

pint_xarray/accessors.py

+        units = either_dict_or_kwargs(units, unit_kwargs, "to")
+
+        return conversion.convert_units(self.ds, units)
+
    def to_base_units(self):


Likewise here?

same here: not sure how to use conversion, but we should definitely add a possibility to only convert certain variables.

TomNicholas · 2020-07-09T13:53:16Z

ah, with the relocation the CI setup is gone. @TomNicholas, do you think you could set this up again?

Thanks for flagging that the CI has gone. I think I've fixed it - @keewis can you just push an empty commit to this branch to see if that triggers the CI on this PR?

keewis · 2020-07-09T14:18:16Z

the CI seems to work again

keewis · 2020-07-09T17:19:31Z

if the code citing style I used (stating the import path) is acceptable, this should be ready to be merged. I'm not quite sure what the best citing style would be, so if we agree on a certain style I'm happy to use that instead.

jthielen · 2020-07-09T17:30:31Z

@keewis Not to get too nit-picky, but I believe the terms of the Apache License require us to include "all copyright, patent, trademark, and attribution notices from the Source form of the Work" along with "a copy of this License", and to also state changes.

So, since we're already including the Apache License (since that is pint-xarray's license as well), we should be fine just including "Copyright 2014-2019, xarray Developers" to that comment to cover the copyright notice requirement and add something like "reused with modification" or whatever other phrasing you find best to cover the stating changes requirement.

keewis · 2020-07-10T13:03:46Z

Not to get too nit-picky

You're not, I just have zero experience with code citing. I looked at the xarray code base for reference, and there the import style reference is mostly used, but then there's also the licenses folder which contains each individual project's license, even if it is a duplicate (there are multiple licenses (MIT?) that are almost identical). And for the register_*_accessor functions, pandas simply states:

# Ported with modifications from xarray
# https://github.com/pydata/xarray/blob/master/xarray/core/extensions.py

while the license is included in their LICENSES folder.

So now, I'm thinking of using this:

# based on xarray.core.utils.either_dict_or_kwargs                                                                                                                                                                                
# https://github.com/pydata/xarray/blob/v0.15.1/xarray/core/utils.py#L249-L268

and including xarray's license in a licenses folder. Not sure if that's enough?

jthielen · 2020-07-10T13:45:27Z

So now, I'm thinking of using this:

# based on xarray.core.utils.either_dict_or_kwargs                                                                                                                                                                                
# https://github.com/pydata/xarray/blob/v0.15.1/xarray/core/utils.py#L249-L268

and including xarray's license in a licenses folder. Not sure if that's enough?

That sounds great except for one thing: it doesn't include the copyright notice (since Apache, unlike BSD and MIT, doesn't include the copyright notice as part of the standard license text). So, I think you'll just have to add that somewhere. At the top of licenses/XARRAY_LICENSE is likely easiest, but perhaps a licenses/README.md or the code comment could be good too.

keewis · 2020-07-10T14:00:09Z

sure, although pandas doesn't seem to have done that (see pandas-dev/pandas@eee83e2)

jthielen · 2020-07-10T14:04:10Z

Hmm...I'm pretty sure then pandas messed up on that one, since the license is clear about requiring "all copyright, patent, trademark, and attribution notices". I'll submit a PR to pandas and see what they say.

keewis · 2020-07-10T14:04:45Z

thanks

keewis · 2020-07-10T14:07:46Z

done. Once you got a response from the pandas devs, we can change this again.

Maybe it's worth pinging @shoyer to resolve this?

keewis · 2020-07-10T14:44:27Z

so if the code citation good enough for now, let's merge?

keewis added 13 commits June 17, 2020 01:07

add a function to attach units to xarray objects

e9f4c91

remove the old and commented out assert_equal_with_units function

d047f0d

add utility functions to attach, convert, extract, and strip units

d2c4a04

add the missing assert functions

f8567c2

reorder the array functions

33ddd39

implement a extract_units function

b1d9268

implement convert_units

00ea3fd

implement attach_units

54f385b

implement strip_units

3101db2

isort and black

b37bdd4

implement DataArray.pint.to

013b056

implement Dataset.pint.to

860aad2

set units as optional

0f076a6

remove a leftover comment about mypy

5b1d334

keewis marked this pull request as ready for review June 29, 2020 15:08

update the minimum required pint version

e3d3e72

TomNicholas approved these changes Jul 8, 2020

View reviewed changes

keewis force-pushed the to branch from a582c57 to a12acd3 Compare July 8, 2020 16:31

TomNicholas mentioned this pull request Jul 8, 2020

API? #6

Open

16 tasks

Merge branch 'master' into to

6855b31

keewis force-pushed the to branch from a12acd3 to 6855b31 Compare July 8, 2020 16:45

Merge branch 'master' into to

3fa00aa

keewis force-pushed the to branch from 71712ba to 3fa00aa Compare July 8, 2020 16:57

keewis added 6 commits July 8, 2020 20:21

don't raise if the default value for units is passed

e970515

undo the test changes

30b75bc

properly separate the conversions

f2c17c3

try separating the example descriptions

2e07ad8

properly refer to pint.Unit

1e20fda

show how to use a dict-like to convert the data

3077b60

fix the docstrings

c875299

jthielen approved these changes Jul 8, 2020

View reviewed changes

TomNicholas mentioned this pull request Jul 9, 2020

Add code coverage to CI #16

Merged

add a comment on the source of the utility function

682ef84

keewis force-pushed the to branch from 603e038 to 682ef84 Compare July 9, 2020 13:54

link to the source and add xarray's license

426c607

add a copyright notice to the xarray license

7ac1c56

jthielen mentioned this pull request Jul 10, 2020

Add xarray copyright notice to comply with reuse under Apache License pandas-dev/pandas#35213

Merged

1 task

jthielen merged commit 4f3b0bc into xarray-contrib:master Jul 10, 2020

keewis deleted the to branch July 10, 2020 15:05

keewis mentioned this pull request Jul 11, 2020

rewrite quantify and dequantify #17

Merged

keewis mentioned this pull request Aug 22, 2020

expose testing functions as public API #24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean-up and implement the conversion methods #11

clean-up and implement the conversion methods #11

keewis commented Jun 27, 2020

keewis commented Jun 28, 2020 •

edited

keewis commented Jun 28, 2020

keewis commented Jul 1, 2020

TomNicholas left a comment

TomNicholas Jul 8, 2020

keewis commented Jul 8, 2020 •

edited

jthielen commented Jul 8, 2020

keewis commented Jul 8, 2020 •

edited

keewis commented Jul 8, 2020 •

edited

keewis commented Jul 8, 2020

jthielen left a comment

jthielen Jul 8, 2020

keewis Jul 9, 2020

jthielen Jul 8, 2020

keewis Jul 9, 2020

TomNicholas commented Jul 9, 2020

keewis commented Jul 9, 2020

keewis commented Jul 9, 2020

jthielen commented Jul 9, 2020 •

edited

keewis commented Jul 10, 2020 •

edited

jthielen commented Jul 10, 2020

keewis commented Jul 10, 2020

jthielen commented Jul 10, 2020

keewis commented Jul 10, 2020

keewis commented Jul 10, 2020 •

edited

keewis commented Jul 10, 2020 •

edited

		pytest.param(None, id="without registry"),
		pytest.param(unit_registry, id="with registry"),

clean-up and implement the conversion methods #11

clean-up and implement the conversion methods #11

Conversation

keewis commented Jun 27, 2020

keewis commented Jun 28, 2020 • edited

keewis commented Jun 28, 2020

keewis commented Jul 1, 2020

TomNicholas left a comment

Choose a reason for hiding this comment

TomNicholas Jul 8, 2020

Choose a reason for hiding this comment

keewis commented Jul 8, 2020 • edited

jthielen commented Jul 8, 2020

keewis commented Jul 8, 2020 • edited

keewis commented Jul 8, 2020 • edited

keewis commented Jul 8, 2020

jthielen left a comment

Choose a reason for hiding this comment

jthielen Jul 8, 2020

Choose a reason for hiding this comment

keewis Jul 9, 2020

Choose a reason for hiding this comment

jthielen Jul 8, 2020

Choose a reason for hiding this comment

keewis Jul 9, 2020

Choose a reason for hiding this comment

TomNicholas commented Jul 9, 2020

keewis commented Jul 9, 2020

keewis commented Jul 9, 2020

jthielen commented Jul 9, 2020 • edited

keewis commented Jul 10, 2020 • edited

jthielen commented Jul 10, 2020

keewis commented Jul 10, 2020

jthielen commented Jul 10, 2020

keewis commented Jul 10, 2020

keewis commented Jul 10, 2020 • edited

keewis commented Jul 10, 2020 • edited

keewis commented Jun 28, 2020 •

edited

keewis commented Jul 8, 2020 •

edited

keewis commented Jul 8, 2020 •

edited

keewis commented Jul 8, 2020 •

edited

jthielen commented Jul 9, 2020 •

edited

keewis commented Jul 10, 2020 •

edited

keewis commented Jul 10, 2020 •

edited

keewis commented Jul 10, 2020 •

edited