hippalectryon-0 / xr-scipy

scipy for xarray eco-system
http://xr-scipy.readthedocs.io
61 stars 9 forks source link

Merge xarray-dsp #5

Closed smartass101 closed 5 years ago

smartass101 commented 5 years ago

Hi, we discussed the possibility of merging https://github.com/smartass101/xarray-dsp into your package in https://github.com/fujiisoup/xr-scipy/issues/4.

At a recent code-camp I and my colleagues prepared the relevant modules for merging with your repo. Don't be discouraged by the large number of commits. This is because I used git subtree to extract the full history from those files, so it will merge the two histories.

We've added proper documentation (without using your docparser for now, might do that later). We will also add some tests later, but it won't be easy to properly test such complex wrappers. For now the Examples serve as a test-run at least.

I had to fix some of your examples and update the Sphinx configuration to build the docs properly.

Let me know what you think.

fujiisoup commented 5 years ago

Oops. Sorry I missed this PR. I'll take a look.

fujiisoup commented 5 years ago

I am not sure if it is good to use the namespace xrscipy.signal for these high-level functions. What I thought when I decided to make xrscipy is that its functions and API should be very similar to the original scipy, so that the readers do not feel any barrier using xrscipy if they are familiar with xarray and scipy.

I still like this concept.

How do you think if we move these high-level and very convenient functions to another namespace, such as xrscipy.signal_filters or any equivalent, but leave xrscipy.signal for counterparts in scipy?

Or is it not confusing if we add scipy's equivalent low-level functions to this namespace later?

fujiisoup commented 5 years ago

Oh, even more important things. I think it would be very nice if we have test cases for these functions, so that we can always make sure very quickly if new change does not break the existing functions.

smartass101 commented 5 years ago

I'm open to become a co-maintainer.

It would be great if you could merge it now so that the new docs are uploaded to RTD (and my colleagues see the fruits of their work)

Do you (or other people you know) still use the package though? Or do you know of some other package into which we should all merge?

Test cases are on my TODO list, for now the Examples kind of serve as a test case. But the complex wrappers will require more thought for testing.

smartass101 commented 5 years ago

I am not sure if it is good to use the namespace xrscipy.signal for these high-level functions. What I thought when I decided to make xrscipy is that its functions and API should be very similar to the original scipy, so that the readers do not feel any barrier using xrscipy if they are familiar with xarray and scipy.

I understand your point, it is indeed hard to judge what is the best course of action in this case. I think the issue stems from the fact that scipy (and specifically its filtering functions) are very low-level with no convenient high-level API, but xarray introduces concepts which are already at a much higher level.

Nevertheless, I believe good documentation and examples lower the barrier more than 1:1 correspondence with SciPy.

How do you think if we move these high-level and very convenient functions to another namespace, such as xrscipy.signal_filters or any equivalent, but leave xrscipy.signal for counterparts in scipy?

Well, savgol_filter is nearly 1:1 as well .... we could keep 1:1 functions in xrscipy.signal (most of the spectral stuff and some filters) and put the rest in specialized modules. But in the end that granularity will likely make it less convenient for users to use.

Perhaps a compromise would be to then import all functions into something like the xrscipy.api or xrscipy namespace for convenience.

I'm afraid convenience is what will drive the usage and adoption of this package, not purity.

Or is it not confusing if we add scipy's equivalent low-level functions to this namespace later?

I don't really think it makes much sense to add the low-level functions at all, because they just create coefficient arrays with no relation to the signals really and make it possible to apply them in specific ways. The main point of these wrappers is to replace a series of ~5 calls with one wrapper function.

fujiisoup commented 5 years ago

Do you (or other people you know) still use the package though? Or do you know of some other package into which we should all merge?

I think I will use it in the future, but currently, my project does not use even scipy.

I'm convinced for the namespace issue, and I'll merge this now.

smartass101 commented 5 years ago

Thank you very much!

I'm convinced for the namespace issue, and I'll merge this now.

Convinced in which way?