Options to binary ops kwargs #1065

chunweiyuan · 2016-10-27T22:17:16Z

Currently the default is join="inner". However, there can be applications where the majority of binary operations require join="outer", not "inner".

Addresses #1059

The solution we come up with is adding a default key/value to OPTIONS, and dynamically access/update it in code (_binary_op in both dataarray.py and dataset.py)

We've also added test modules in test_dataarray.py and test_dataset.py. All tests passed.

…th a test module in test_dataarray.py

shoyer · 2016-10-28T01:15:30Z

Let's find a more descriptive name for this option than merely join. Maybe: compute_join, compute_align, arithmetic_join, math_join, ...?

Also please add documentation notes about this new functionality to this section of the docs and the "what's new" page.

shoyer

Your tests look like they cover the bases, but they are a bit long. More minimal (e.g., 1D array) test cases would be better because they have a higher signal to noise ratio. Another option would be reuse existing test datasets (e.g., create_data_test()), since the actual values don't matter too much here. We already have plenty of other tests that verify that align works properly on higher dimensional data, so there's value in keeping these simple.

shoyer · 2016-10-28T01:16:14Z

xarray/core/dataarray.py

        @functools.wraps(f)
        def func(self, other):
            if isinstance(other, (Dataset, groupby.GroupBy)):
                return NotImplemented
            if hasattr(other, 'indexes'):
-                self, other = align(self, other, join=join, copy=False)
+                # if user does not specify join, default to OPTIONS['join']


This comment just states what the code does, so it would be better to leave it out

shoyer · 2016-10-28T01:16:36Z

xarray/core/dataarray.py

        @functools.wraps(f)
        def func(self, other):
            if isinstance(other, (Dataset, groupby.GroupBy)):
                return NotImplemented
            if hasattr(other, 'indexes'):
-                self, other = align(self, other, join=join, copy=False)
+                # if user does not specify join, default to OPTIONS['join']
+                how_to_join = join or OPTIONS['join']


Use if join is None instead of implicitly testing join.

This would trigger an UnboundLocalError:
if join is None: join = OPTIONS['join']
because join is not defined in the local scope prior to that line.

So the choice is either
1.) if join is None:
how_to_join = OPTIONS['join']
else:
how_to_join = join
or
2.) how_to_join = join or OPTIONS['join']

I opted for (2) for simplicity. Is the style of (1) generally preferred here?

You could do the if/else inline, if you prefer:
how_to_join = OPTIONS['join'] if join is None else join

But yes, I do prefer explicitly checking against None for default values rather than relying on implicit falseness of None.

Funny, your suggestion is the same as my first edit to the code, but then my teammates suggested I should use the other options.

Nomenclature is not my forte. I'll make the default
OPTIONS = {'display_width': 80, 'align_type': "inner"}
and change your suggestion to the following

align_type = OPTIONS['align_type'] if join is None else join self, other = align(self, other, join=align_type, copy=False)

if there's no objection.

We also align in other operations, e.g., merge, for which the default is to do an outer join. So the option name should be specific to computation/arithmetic.

binary_op_align_type?

alright, I'm using arithmetic_join....

Yeah I think arithmetic_join or compute_join is probably the way to go. We will probably also use this when/if we ever add ternary operations, too.

shoyer · 2016-10-28T01:17:34Z

xarray/test/test_dataarray.py

+        A test method to verify the ability to set binary operation join kwarg
+        ("inner", "outer", "left", "right") via xr.set_options().
+        """
+        # First we set up a data array


These sort of comments (repeating the code) just add noise

shoyer · 2016-10-28T01:19:43Z

xarray/test/test_dataarray.py

+        # create another data array with the last z slice missing
+        missing_last_z = arr[:,:,0:-1].copy()
+        # because the default in OPTIONS is join="inner", we test "outer" first
+        xr.set_options(join="outer")


Use the context manager for tests instead (e.g, with xr.set_options(join='outer'):. It cleans itself up if an exception is raised in the middle of the test, which makes it more hygenic. Otherwise, if the next assert fails it would also trigger a bunch of other failing tests when the option doesn't get reset.

shoyer · 2016-10-28T01:21:13Z

xarray/test/test_dataarray.py

+        # because the default in OPTIONS is join="inner", we test "outer" first
+        xr.set_options(join="outer")
+        result = missing_last_x + missing_last_z
+        self.assertTrue(result.shape == arr.shape)


Instead of checking specific properties, construct the full expected result (e.g., by calling align with join='outer' on the inputs) and use assertDataArrayEqual to verify that expect result is the result you get.

shoyer · 2016-10-28T01:25:02Z

xarray/test/test_dataset.py

+        self.assertTrue(result.foo.notnull().all())
+        self.assertFalse('c' in list(result.foo['x']))
+        self.assertFalse(2 in list(result.foo['z']))
+        # just for kicks, what happens when the dataarrays have different names?


We already have tests for similar behavior -- see test_dataset_math_auto_align. This might belong there (more tests are great!), but it doesn't belong here.

It does raise the interesting question of what the behavior should be when join='outer' for Dataset objects with different data variables. Should the result now have the union of data variable names? If so, this will take a bit more retooling. (This would be easier after #964 is merged)

shoyer · 2016-10-28T01:31:55Z

xarray/test/test_dataarray.py

+
+    def test_binary_op_join_setting(self):
+        """
+        A test method to verify the ability to set binary operation join kwarg


Usually we don't put docstrings on test methods

Cool. Will remove.

chunweiyuan · 2016-10-28T18:58:42Z

Out of curiosity, how do I test inner-join vs. outer-join using only 1-D arrays? I can certainly simplify the current tests to be 2D + 1D.

shoyer · 2016-10-28T19:03:56Z

Set coordinates on the 1D -- something like [0,1,2] for the first, [1,2,3]
for the second. Then the inner join should have [1,2] vs [0,1,2,3] for
outer join.
On Fri, Oct 28, 2016 at 12:00 PM chunweiyuan notifications@github.com
wrote:

Out of curiosity, how do I test inner-join vs. outer-join using only 1-D
arrays? I can certainly simplify the current tests to be 2D + 1D.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1065 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABKS1qyX-HigWTUAJnBuCpR4AqNFzyYaks5q4kXigaJpZM4Ki4oh
.

…ed to computation.rst and whats-new.rst

shoyer · 2016-10-28T22:39:24Z

xarray/test/test_dataarray.py

+        dim = 'x'
+        align_type = "outer"
+        coords_l, coords_r = [0, 1, 2], [1, 2, 3]
+        missing_0 = xr.DataArray(coords_l, [(dim, coords_l)])


These names look backwards? missing_0 currently has labels [0,1,2].

shoyer · 2016-10-28T22:41:39Z

xarray/test/test_dataarray.py

+            experimental = missing_0 + missing_3
+            missing_0_aligned, missing_3_aligned =\
+                xr.align(missing_0, missing_3, join=align_type)
+            control = missing_0_aligned + missing_3_aligned


could also explicitly set control = xr.DataArray([np.nan, 2, 4, np.nan], [(dim, [0, 1, 2, 3])]), which is actually even better than using align and arithmetic here

shoyer · 2016-10-28T22:41:59Z

xarray/test/test_dataarray.py

+        missing_0 = xr.DataArray(coords_l, [(dim, coords_l)])
+        missing_3 = xr.DataArray(coords_r, [(dim, coords_r)])
+        with xr.set_options(arithmetic_join=align_type):
+            experimental = missing_0 + missing_3


Drop out of the set_options block after this line.

shoyer · 2016-10-28T22:46:02Z

doc/computation.rst

+
+.. ipython:: python
+
+    xr.set_options(arithmetic_join="outer")


Can you only show the example using with?

I'm now wondering if we need add some mechanism to require using the context manager for arithmetic_join. If users don't use a context manager, very mysterious/nonlocal bugs could appear when this changes the behavior of unrelated code (e.g., in an import statement).

For our use case we would set arithmetic_join="outer" from the get-go and run all of our binary calculations like that throughout our simulation. We'd use that as our default, and maybe occasionally switch to other join types. Requiring context manager every time we deviate from "inner" seems onerous.

I removed the permanent change example and left only the context manager example in there. I'm not sure what the best compromise is. Perhaps we could print some warning message when it's permanently set?

shoyer · 2016-10-29T01:10:18Z

It occurs to me now that the other reason why people avoid global variables is because of concerns around thread safety and distributed computation. Even with the context manager, bad things will happen if you use set_options simultaneously from different threads.

I need to think about this a little more. Copious warnings in the docs might be enough, or maybe entering a set_options context should also enter an explicit threading.Lock() context.

On the other hand, the current implementation is basically identical to how functionality like NumPy's np.errstate() works, and people seem to be pretty happy with that.

@mrocklin does this seem like the sort of change we can do in xarray without seriously regretting it later? Xarray uses basically the same set_options machinery that dask has.

mrocklin · 2016-10-29T01:36:05Z

I haven't noticed people getting caught by using set_options from multiple threads. It definitely isn't threadsafe but it's usually only used when constructing computations, which is often done from a single thread.

It would be good to ensure that this affects the graph and that the actual tasks don't depend on the global state.

shoyer · 2016-10-29T01:40:11Z

It would be good to ensure that this affects the graph and that the actual tasks don't depend on the global state.

Indeed, we are safe here. align is turned into a bunch of NumPy or dask.array calls, so this option will simply effect how the graph is built.

chunweiyuan · 2016-11-10T17:15:29Z

Just checking in. Is there something else that needs to be added to this PR before merge?

shoyer · 2016-11-10T17:45:11Z

My concern is how this setting currently does not change how we join variables in Dataset math. Consider the current behavior -- it always does an inner join of the data variables:

In [2]: import xarray as xr

In [3]: ds1 = xr.Dataset({'foo': 1, 'bar': 2})

In [4]: ds2 = xr.Dataset({'bar': 2, 'baz': 3})

In [5]: ds1
Out[5]:
<xarray.Dataset>
Dimensions:  ()
Coordinates:
    *empty*
Data variables:
    bar      int64 2
    foo      int64 1

In [6]: ds2
Out[6]:
<xarray.Dataset>
Dimensions:  ()
Coordinates:
    *empty*
Data variables:
    bar      int64 2
    baz      int64 3

In [7]: ds1 + ds2
Out[7]:
<xarray.Dataset>
Dimensions:  ()
Coordinates:
    *empty*
Data variables:
    bar      int64 4

Logically, it makes sense for this option to switch this to an outer join instead.

chunweiyuan · 2016-11-10T23:48:11Z

Alright, if that's desired, I'd just change code in apply_over_both() of dataset._calculate_binary_op to react to different kwarg values, and adding a method in test_dataset.py. Should be able to push before the weekend is over.

shoyer · 2016-11-11T23:36:20Z

xarray/core/options.py

@@ -26,6 +26,10 @@ class set_options(object):
    """
    def __init__(self, **kwargs):
        self.old = OPTIONS.copy()
+        for key in kwargs:


please rebase or merge master -- I actually already added in something very similar
https://github.com/pydata/xarray/blob/master/xarray/core/options.py

shoyer · 2016-11-11T23:39:04Z

xarray/core/dataset.py

-                elif fillna:
-                    # this shortcuts left alignment of variables for fillna
+
+            for k in set(lhs_data_vars) & set(rhs_data_vars):


It is important to maintain the order of data variables: should be lhs first, then rhs only variables.

So, probably should use a loop over lhs_data_vars, followed by rhs_data_vars.

shoyer · 2016-11-11T23:41:43Z

xarray/core/dataset.py

+            for k in set(lhs_data_vars) & set(rhs_data_vars):
+                dest_vars[k] = f(lhs_vars[k], rhs_vars[k])
+            if join in ["outer", "left"]:
+                for k in set(lhs_data_vars) - set(rhs_data_vars):
                    dest_vars[k] = lhs_vars[k]


This was only the right default for fillna. For other operations, should be NaN instead (or, more specifically, the result of f(lhs_vars[k], np.nan).

shoyer · 2016-11-11T23:42:48Z

xarray/test/test_dataset.py

+        self.assertDatasetEqual(actual, expected)
+
+        with xr.set_options(arithmetic_join='outer'):
+            expected = xr.Dataset({'foo':1, 'bar': 4, 'baz': 3})


As noted above, should be xr.Dataset({'foo': nan, 'bar': 4, 'baz': nan}) for consistent outer join semantics

…ult filler.

shoyer · 2016-11-12T03:26:20Z

xarray/core/dataset.py

+                if k in rhs_data_vars:
+                    dest_vars[k] = f(lhs_vars[k], rhs_vars[k])
+                elif join in ["left", "outer"]:
+                    dest_vars[k] = lhs_vars[k] if fillna else\


per PEP8, use parenthesis to join across multiple lines instead of \

shoyer · 2016-11-12T03:27:41Z

xarray/test/test_dataset.py

+
+
+
+


too much white space here -- by convention, leave only 2 blank lines

shoyer · 2016-11-12T03:28:08Z

xarray/test/test_dataarray.py

+        missing_0 = xr.DataArray(coords_r, [(dim, coords_r)])
+        with xr.set_options(arithmetic_join=align_type):
+            actual = missing_0 + missing_3
+        missing_0_aligned, missing_3_aligned =\


line break after xr.align( instead of using the explicit \

shoyer · 2016-11-12T03:28:32Z

xarray/core/options.py

@@ -1,7 +1,8 @@
 from __future__ import absolute_import
 from __future__ import division
 from __future__ import print_function
-OPTIONS = {'display_width': 80}
+OPTIONS = {'display_width': 80,
+           'arithmetic_join': "inner"}


 class set_options(object):


can you update the docstring for this class?

shoyer · 2016-11-12T03:59:56Z

Thank you @chunweiyuan!

chunweiyuan · 2016-11-12T04:00:10Z

woohoo!

Chun-Wei Yuan added 3 commits October 25, 2016 21:18

Added join key to OPTIONS, used in dataarray & dataset binary ops, wi…

3b04fb0

…th a test module in test_dataarray.py

Added binary_ops test to test_dataset.

643fa49

Changed variable names according to review comments.

43df4d0

shoyer mentioned this pull request Oct 28, 2016

New function for applying vectorized functions for unlabeled arrays to xarray objects #964

Merged

shoyer reviewed Oct 28, 2016

View reviewed changes

Changed default key to arithmetic_join, and shortened tests. Also add…

ebb2ad0

…ed to computation.rst and whats-new.rst

shoyer reviewed Oct 28, 2016

View reviewed changes

Emphasis on context manager for xr.set_options() use.

ab717fa

Changed to actual vs expected testing nomenclature.

8aa969b

Chun-Wei Yuan added 2 commits November 11, 2016 14:22

Applies join options to Dataset.data_vars as well.

46ad36a

Fixed merge conflict in options.py

4a05ca4

shoyer reviewed Nov 11, 2016

View reviewed changes

Preserve order of joined data_vars, left-to-right. Use np.nan as defa…

a7b7497

…ult filler.

shoyer reviewed Nov 12, 2016

View reviewed changes

PEP8 and doctring.

88c37bc

shoyer merged commit 95bca62 into pydata:master Nov 12, 2016

chunweiyuan deleted the options_to_binary_ops_kwargs branch November 12, 2016 04:17

Options to binary ops kwargs #1065

Options to binary ops kwargs #1065

Conversation

chunweiyuan commented Oct 27, 2016 • edited Loading

shoyer commented Oct 28, 2016

shoyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunweiyuan Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunweiyuan commented Oct 28, 2016

shoyer commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Oct 29, 2016

mrocklin commented Oct 29, 2016

shoyer commented Oct 29, 2016

chunweiyuan commented Nov 10, 2016

shoyer commented Nov 10, 2016

chunweiyuan commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Nov 12, 2016

chunweiyuan commented Nov 12, 2016

chunweiyuan commented Oct 27, 2016 •

edited

Loading

chunweiyuan Oct 28, 2016 •

edited

Loading