Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a "labels" field in AxisInfo if present. #1

Merged
merged 8 commits into from
Aug 6, 2024

Conversation

cboulay
Copy link
Contributor

@cboulay cboulay commented Jun 13, 2024

(Migrating from ezmsg-org/ezmsg#123)

ezmsg-lsl has a SpaceAxis that I patch into AxisArray, and SpaceAxis has a labels field. This solves an issues that we discussed in ezmsg-org/ezmsg#43

Units that transform the AxisArray in a way that modifies the length of the dimension with the "labels" field, should probably also modify the labels, if present! This PR does that for Units that I'm aware of that modify the shape of the array along axes other than "time".

For affine_transform, it's difficult to know what the new labels should be so in most cases they are just dropped. However, if the affine_transform has rows or columns of zeros then we can get a good guess at thew new labels.

… array shape along dimension with labels must also modify labels field.
* faster unit tests on core processing
* accommodate size-0 arrays
* slight optim. using template struct.
@cboulay
Copy link
Contributor Author

cboulay commented Jun 13, 2024

From @griffinmilsap here

I didn't know about an lsl connector; this is really cool! When you're ready, let's add it into the README as another extension :D

AxisArray is VERY inspired by XArray (my absolutely favorite data structure for offline analyses) that unfortunately has a little too much going on under the hood to be performant enough for low latency, high-message-rate stream processing. An earlier version of ezmsg-sigproc actually used DataArrays for the messages and that was a lovely developer experience even if my CPUs pegged processing 8 channels of 250 Hz EEG data -- but I digress.

I feel like the general solution to this particular problem is adding a coords implementation into AxisArray that captures some core set of functionality that we find in XArray's DataArray.coords. I wrote the majority of the AxisArray implementation in an afternoon when I was pressured by an incoming deadline. I spent about an hour thinking about how to get coords in without destroying performance or my guiding mantra that "AxisArray is simply a metadata wrapper around a numpy array". I came to the conclusion at that time that a limited coords functionality in AxisArray would be 1. incredibly useful, and 2. performant if we removed all the integrity checks/safety rails that XArray's implementation has. I simply didn't have the bandwidth (at that time) to even do a first-pass implementation.

In summary, AxisArray needs some TLC for

  1. Better Typehinting (even simple fixes like sel/isel returning subclasses of AxisArray, not just AxisArrays)
  2. Better dev experience/consistency (why is concatenate a staticmethod?)
  3. A lightweight coords implementation

Coming back to today and this PR: These operations on coords/labels probably eventually belong in AxisArray helper functions/implementation rather than spread throughout the signal processing modules. Collecting common operations on AxisArrays together in one place would make this whole thing a lot easier to maintain, but if this gets you where you're going today I support a merge after review (feel free to merge it in yourself once my review posts)

@cboulay cboulay merged commit e4cd550 into dev Aug 6, 2024
@cboulay cboulay deleted the cboulay/axarr_labels branch August 6, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant