search

hippalectryon-0 / xr-scipy

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

move xrscipy.other.signal to xrscipy.signal as it was before #36

Closed smartass101 closed 1 year ago

smartass101 commented 1 year ago

I appreciate you took over the maintenance burden @hippalectryon-0, I really do. Is there a good reason why you moved the signal wrappers into the "other" subpackage? They do follow the scipy naming, so putting them under other is rather confusing for users, especially to those who used the older xrscipy.signal module and now wonder where it went.

hippalectryon-0 commented 1 year ago

The functions put in xrscipy.other are meant to be functions that don't have an equivalent in scipy.

Concerning xrscipy.other.signal, we find a mix of scipy.signal functions (csd, spectrogram, coherence, hilbert) and functions not found in xrscipy.signal (crossspectrogram, freq2lag, xcorrelation, psd, coherogram), and given the current balance I previously chose to put everything in xrscipy.other.

Do you think it would be better to split the package and put csd, spectrogram, coherence, hilbert in xrscipy.signal while keeping the others as-is in xrscipy.other.signal ?

smartass101 commented 1 year ago

Hi, thanks for the explanation, I see your point. By the way, psd is actually welch in scipy nomenclature, but you're right, I should at least add an alias.

The others like crossspectrogram, coherogram are actually just a (IMHO) better (more complete) API to scipy.signal._spectral_helper, because the current SciPy API is quite limiting as it does expose that function directly. All of the various spectral analysis functions are mostly just specific sets of arguments to that generic spectral analysis function and the combination of the results. Ideally, Scipy could make this helper function public and improve the API, but it might take a long time before that happens. For now I suggest this approach: Expose the wrapper for this generic function in xrscipy.signal directly and perhaps split off some of the extra functions wrapping a specific call to this function to something like xrscipy.signal.extra.

And then there are some convenient helpers like freq2lag, xcorrelation which indeed do not have a direct Scipy equivalent. The proper way would be to put them upstream, for now I suggest they could be in xrscipy.signal.extra.

hippalectryon-0 commented 1 year ago

Can you check if https://github.com/hippalectryon-0/xr-scipy/releases/tag/2.0.0.pre does what you wanted ?

smartass101 commented 1 year ago

Thanks, that looks good. I actually had in mind really moving the "extra" functions to separate modules, but I think your approach of just splitting the namespaces is a better compromise in terms of maintainability. Truly splitting stuff into separate non-extra and extra modules + sharing the helpers in between would be possibly more difficult to maintain. I just feel I should point out in the interest of clarity that in principle just importing anything from filters (i.e. even if just using the non-extra namespace) registers the `.filt accessor https://github.com/hippalectryon-0/xr-scipy/blob/d1bd88a284a2ddcfe74b04c7ed7ae28cef2e487f/xrscipy/signal/filters.py#L556

hippalectryon-0 commented 1 year ago

ok ! I'll make an official release in a few days.