Thank you for continuing this discussion even though you didn't agree with the initial proposal. I have accepted and embraced option 3) as it is indeed about the cleanest and most readable option.

Sorry for chiming in here. Using the example above, if we have the choice between: 1) sig.lowpass(0.2).multiply(3) 2) sig.accessorlib.lowpass(0.2).multiply(3) 3) sig.pipe(xdsp.lowpass, 0.2).pipe(np.multiply, 3) 4) sig(xdsp.lowpass, 0.2)(np.multiply, 3)

Of course, (1) is attractive because straight forward (as the dev of a small xarray accessor, I also forget very often that I have to add an attribute between the dataset and my function call). But (2) has the huge advantage that it clearly says where the code of the function is found, and where to ask questions when things do not work as expected.

(3) is OK for me since it is very explicit, but I find that (4) is quite ugly.

I think we probably need to agree to disagree here. I will update the docs in response to feedback (which is greatly appreciated!) and when I do so I will close out this issue.

Perhaps I will suggest DataArray.__call__ = DataArray.pipe (maybe that could be added in xarray ?

This seems too magical to me, but you are welcome to make another issue to see what others think. __call__ is not searchable in the way the .pipe is.

Code is read in text form more often than it is interactively explored.

Good point, in that case explicit namespacing indeed helps.

At Google, our Python style guide actually prohibits writing import like from xarray import Dataset. You have to write import xarray or import xarray as xr and always use the namespace.

A module-level namespace has nothing to do with the class namespace, but I see you try to tie them, which makes sense in relationship with the argument about reading code in text form. However, that may not be clear for Python programmers as those namespaces are not tied in reality, better mention it in the docs. BTW, if you are enforcing some specific style guide, please note that in the docs. And I hope you strike the right balance between style complacency and universality.

xarray objects are already non-threadsafe, in the same way that the built-in list and dict are not threadsafe. I don't see how caching attributes changes this. You can choose whether or not to save state on the accessor (and of course, generally it would be better not to).

My problem with non-functional paradigms lies more in the apply, map... paradigms which accessors don't fit into than thread safety.

Finally, I'll note that we also have the .pipe method (e.g., array.pipe(square)), so if you just want functions that you can call with method chaining syntax, you don't even need to write an accessor at all.

That is indeed a good alternative, just not sure my colleagues will like the transition from sig.lowpass(0.2).multiply(3) to sig.pipe(xdsp.lowpass, 0.2).pipe(np.multiply, 3). A benefit of pipe is that methods can be tab-completed from namespaces (useful for interactive usage) and that any compatible function can be used, not just registered methods. Perhaps I will suggest DataArray.__call__ = DataArray.pipe (maybe that could be added in xarray ? should I make an issue for that?) which would make it quite convenient to write only sig(xdsp.lowpass, 0.2)(np.multiply, 3) which is almost the same in terms of chars written and has quite clear syntax (calling a signal with a function argument applies the function to it).

Python's introspection capabilities are powerful enough that users can find out such information.

I don't really agree here. Code is read in text form more often than it is interactively explored.

At Google, our Python style guide actually prohibits writing import like from xarray import Dataset. You have to write import xarray or import xarray as xr and always use the namespace.

And then there"s also the problem of accessors not being usable in functional programming paradigms.

xarray objects are already non-threadsafe, in the same way that the built-in list and dict are not threadsafe. I don't see how caching attributes changes this. You can choose whether or not to save state on the accessor (and of course, generally it would be better not to).

Finally, I'll note that we also have the .pipe method (e.g., array.pipe(square)), so if you just want functions that you can call with method chaining syntax, you don't even need to write an accessor at all.

You are certainly welcome to monkey patch -- that's the Python philosophy, after all -- but I'm not going to recommend it or make it easy. But I would even subclassing before considering monkey patching -- at least then your methods are contained to your own code, instead of contaminating a global namespace.

The namespace argument doesn't seem very convincing since you already implement many methods which may shadow variables (mean, diff). By limiting control of the namespace you make some uses somewhat inconvenient. If you want users to use DataArray as a general and universal and also extensible container, limiting its namespace goes against that. If they shadow vars by their methods, that's their decision to make.

While it may seem cleaner to have a stricter API, in real use cases users care more about convenient code access than where it came from. And when they look at the method object it will clearly tell them where it was defined. Python's introspection capabilities are powerful enough that users can find out such information.

What I meant by the 2. point was that in many cases one just needs a simple method and with the accessor approach one has to write extra lines of code like the ones you suggested earlier that may later seem cryptic. Caching of the accessor can be indeed useful, just not always. If you want people to develop plugins, make it as simple as possible and yet also advanced for those who require it. And then there"s also the problem of accessors not being usable in functional programming paradigms.

Tl;dr: accessors have benefits (namespace containment, caching) but also limitations (not functional paradigm, overkill sometimes). Give users more control over methods and you'll get more plugins.

On November 6, 2016 2:22:44 PM GMT+01:00, Stephan Hoyer notifications@github.com wrote:

Is it because of namespace growth/conflicts? There are already many methods like diff, any which don't seem particularly more important than others. For instance, ndarray has no diff method yet you implement it.

Indeed. My thinking was the xarray.Dataset and xarray.DataArray are in the "xarray" namespace. We allow you to register an extension namespace, but want to keep it well contained and under one attribute, so it's clear(er) to users and developers what is going on, and where the code comes from.

A stricter approach would have been to put everything under an attribute just for extensions, e.g., Dataset.x.namespace instead of Dataset.namespace, but this gets even more cumbersome -- and also conflicts with variables named x!

Could you please give some clear arguments why you discourage the use of normal methods? The two arguments listed in the docs don't really make a compelling case against method monkey-patching, because 1. name clashes can be easily checked for either approach (in either case you just check the existence of a class attribute)

I'll add a wrote about the value of namespaces to the doc.

1. caching on the dataset sometimes makes no sense and just adds redundancy and complicates the design and registering of extra functionality

We could certainly turn this off (optionally) if there are cases where it does the wrong thing. Could you go into this in a little more detail, perhaps with a concrete example? My expectation was that this should have minimal design or performance downsides.

You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/pydata/xarray/issues/1080#issuecomment-258680571

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Is it because of namespace growth/conflicts? There are already many methods like diff, any which don't seem particularly more important than others. For instance, ndarray has no diff method yet you implement it.

Indeed. My thinking was the xarray.Dataset and xarray.DataArray are in the "xarray" namespace. We allow you to register an extension namespace, but want to keep it well contained and under one attribute, so it's clear(er) to users and developers what is going on, and where the code comes from.

A stricter approach would have been to put everything under an attribute just for extensions, e.g., Dataset.x.namespace instead of Dataset.namespace, but this gets even more cumbersome -- and also conflicts with variables named x!

Could you please give some clear arguments why you discourage the use of normal methods? The two arguments listed in the docs don't really make a compelling case against method monkey-patching, because 1. name clashes can be easily checked for either approach (in either case you just check the existence of a class attribute)

I'll add a note about the value of namespaces to the doc.

1. caching on the dataset sometimes makes no sense and just adds redundancy and complicates the design and registering of extra functionality

We could certainly turn this off (optionally) if there are cases where it does the wrong thing. Could you go into this in a little more detail, perhaps with a concrete example? My expectation was that this should have minimal design or performance downsides.

Thank you for your response.

I still don't understand why you are pushing accessors in place of methods to such an extent. Is it because of namespace growth/conflicts? There are already many methods like diff, any which don't seem particularly more important than others. For instance, ndarray has no diff method yet you implement it.

While the solutions you presented are usable, they seem like workarounds and somewhat redundant or add extra like overhead (in terms of writing code). Registering extra dataset accessors where DataArray method application would do seems again redundant.

I would definitely discourage writing too many of such methods, though.

Could you please give some clear arguments why you discourage the use of normal methods? The two arguments listed in the docs don't really make a compelling case against method monkey-patching, because 1. name clashes can be easily checked for either approach (in either case you just check the existence of a class attribute) 2. caching on the dataset sometimes makes no sense and just adds redundancy and complicates the design and registering of extra functionality

I'm not trying to say that the accessor approach is wrong, I'm sure it makes sense for certain plugins. I'm just trying to share my experience with a very similar case where the simpler method approach turned out to be satisfactory and I think enabling it would increase the chances of more xarray plugins (which may not need accessor logic) coming to life.

Btw, perhaps it might be better to (perhaps optionally) issue a warning when overriding an existing class attribute during registering instead of completely refusing to do so.

I don't see a conflict between encouraging accessors and making duplicate methods/functions. If you want duplicate method/functions, you can totally do that on an accessor class:

``` python import functools import xarray

@xarray.register_dataarray_accessor('custom') class CustomAccessor(object): def init(self, data_array): self._data_array = data_array

def patch_custom(func): @functools.wraps(func) def method(accessor, args, kwargs): return func(accessor._data_array, args, **kwargs) setattr(CustomAccessor, func.name, method) return func

@patch_custom def square(data_array): return data_array ** 2

xarray.DataArray([1, 2, 3], dims='x').custom.square()

<xarray.DataArray (x: 3)>

array([1, 4, 9])

Coordinates:

* x (x) int64 0 1 2

```

If you really desire, you can even make method-like accessors by adding a __call__ method, e.g.,

@xarray.register_dataarray_accessor('square') class Square(object): def __init__(self, data_array): self._data_array = data_array def __call__(self): return self._data_array ** 2

or even simpler

@xarray.register_dataarray_accessor('square') def square_accessor(data_array): def square(): return data_array ** 2 return square

I would definitely discourage writing too many of such methods, though.

Finally, it seems like the simplest solution to your concern about needing methods for Dataset.apply would be to register an accessor for Dataset as well, with register_dataset_accessor.