ENH: finite difference method diff

[andreas-h]

adds diff method to both DataArray and Dataset. closes #490

Still to be done:

• tests

Possible enhancements:

• allow numeric axis instead of dim for DataArray objects. The signature would then be DataArray.diff(dim=None, n=1, axis=None) and I would need to check that exactly one of dim and axis is None. I find it a bit ugly; DataArray.diff(n=1, dim=None, axis=None) would be nicer in my view. But then the signatures for DataArray.diff and Dataset.diff would be different, which is also not nice.
• allow specifying the new coordinate array explicitly via a coord kwarg instead of just taking the coordinate values of the upper bounds.

What do you think?

[andreas-h]

Actually, the current implementation leads to 0.0 for arrays inside a Dataset which don't have dim as a dimension. I personally would find it more intuitive if those arrays would not be touched at all.

Do you agree, or do you prefer this 0.0 for unaffected arrays?

[shoyer]

This looks very nice -- thanks for putting together the PR!

allow numeric axis instead of dim for DataArray objects.

I wouldn't bother with this. It's not so elegant, and dim already takes care of all the desired functionality.

allow specifying the new coordinate array explicitly via a coord kwarg instead of just taking the coordinate values of the upper bounds.

I wouldn't allow full control here, but maybe a keyword argument for choosing whether to take the "lower", "upper" or "mean" labels would be appropriate.

Actually, the current implementation leads to 0.0 for arrays inside a Dataset which don't have dim as a dimension. I personally would find it more intuitive if those arrays would not be touched at all.

I agree. It would be better to skip those variables entirely, like the current behavior for aggregation functions like mean.

[shoyer]

nit: one extra pair of parenthesis around n

[andreas-h]

I wouldn't allow full control here, but maybe a keyword argument for
choosing whether to take the "lower", "upper" or "mean" labels would be
appropriate.

I don't see how 'mean' could work, as the coord might be of dtype str.
But I'm implementing 'upper' and 'lower'. If the user wants something
different, she can always just swap the coord manually.

[andreas-h]

I'm not really happy with my implementation; the coordinate/variable handling in lines 1870-1884 are far from elegant. But is there a nicer way to do this?

[shoyer]

I would try:

kwargs_new = kwargs_end if coord_new == 'upper' else kwargs_start

for name, var in iteritems(self.variables):
    if dim in var.dims:
        if name in self.data_vars:
            variables[name] = var.isel(**kwargs_end) - var.isel(**kwargs_start)
        else:
            # don't do arithmetic on coordinates
            variables[name] = var.isel(**kwargs_new)
    else:
        # these variables should be unchanged
        variables[name] = var

# this private constructor preserves existing coordinates
# it's also much faster, because it doesn't need to do validation
difference = self._replace_vars_and_dims(variables)

[andreas-h]

this private constructor preserves existing coordinates

it's also much faster, because it doesn't need to do validation

difference = self._replace_vars_and_dims(variables)

_replace_vars_and_dims gave me an error because the size of the
dimension on which I do the diff is changing. Was I using it wrongly?

[shoyer]

Let me test your PR and debug this. My guess is that some of the variables were ending up with inconsistent dimension sizes (the Dataset constructor resolves that by doing an outer join of the index labels).

[shoyer]

I don't see how 'mean' could work, as the coord might be of dtype str.
But I'm implementing 'upper' and 'lower'. If the user wants something
different, she can always just swap the coord manually.

Indeed, this would fail for string dtypes. I can see some possible utility in centered coordinates, especially for second order differences. We can certainly leave this for later, though.

[shoyer]

Just made a PR against your branch with my suggestion: https://github.com/andreas-h/xray/pull/1

It looks like it's working now.

[shoyer]

This could also use a bit of documentation -- at least mention on "What's New" and in the API docs.

[shoyer]

please add a test that catches each of these exceptions

[shoyer]

@andreas-h we're going to release v0.6 in the next day or two. I'd love to include this enhancement in it if you have the time to finish it up :).

[andreas-h]

That's strange, I cannot reproduce the test failure on my system ... Any ideas?

[shoyer]

See http://stackoverflow.com/questions/19668395/str-format-for-python-2-6-gives-error-where-2-7-does-not

[shoyer]

Could you try doing rebase -i master to pick out your commits? Otherwise github shows everything since the merge (look at the commit tab on this PR).

[shoyer]

looks like we still need a unit test for label='lower'

[shoyer]

OK, merging.

Thank you!