search

ezmsg-org / ezmsg

Pure-Python DAG-based high-performance SHM-backed pub-sub and multi-processing pattern
https://ezmsg.readthedocs.io/en/latest/
MIT License
9 stars 5 forks source link

Request: sigproc Spectrogram Collection #44

Closed cboulay closed 4 months ago

cboulay commented 8 months ago

I think it's a pretty common use-case to put a Window in front of Spectrum. This should probably be in a pre-made collection with easy-access settings.

(1-of-several requests to get at SpectralBandpower)

cboulay commented 8 months ago

While this is easy enough to do on my own, especially following the example in ezmsg-panel, one of the design issues I encountered was that of the axes. The way I'm doing it currently is to have Window create a new "step" axis. Spectrum acts on the "time" axis and replaces it with a "freq" axis, so I'm left with "ch", "freq", and "step". Many downstream steps, like EWMFilter, default to operating on the "time" axis so I'm forced to configure them to operate on the "step" access instead. This isn't that big of a deal, but it might be more convenient and intuitive if the "step" axis were renamed to "time" after Spectrum.

These kinds of design questions I think are best solved in collaboration, instead of each user rolling their own.

griffinmilsap commented 7 months ago

@cboulay Sorry for the delay in response on these diligent issue reports, I was taking a short break. Agreed that we should have a dedicated collection; unclear to me what good default names are for these axes. As you noted, my ezmsg-panel implementation uses a "win" dimension, and the default names need to be changed from there on forward.

I think it might be a bit surprising for the spectrum unit to rename the requested axis to a time axis without being explicitly told to do so; I actually think the core issue is with having a default axis in the existing sigproc units; which was only done for backward compatibility. Perhaps we just deprecate the default axis names and move on?

cboulay commented 7 months ago

Sorry I didn't catch this response until just now.

By deprecating the default axis names, would None still be supported and assume 0th axis? Or would axis become a required setting?