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

windowing generator now yields AxisArray. #103

Merged
merged 2 commits into from
May 11, 2024

Conversation

cboulay
Copy link
Collaborator

@cboulay cboulay commented Mar 3, 2024

Fixes #100

The windowing generator now always yields an AxisArray.

Previously, the return type was List[AxisArray]. Depending on the configuration, the list might have always been length 1 (in 1:1 mode) or usually 1, unless a big chunk came in and multiple windows were available.
Now, there is always a newaxis added ("win" by default) and that will usually have length 1, but sometimes more.

The "win" axis might also have length 0 when the most recent send was inadequate to fill the next window. Previously, an unfilled .send would yield None (previous yield type incorrectly failed to include Optional[...]). This is probably the biggest change here and might merit some discussion:
What should generators yield when the .send does not provide enough data to yield a result? None, an empty AxisArray, or an AxisArray with most details correct except one dimension is 0 and msg.data.size==0?

The Unit still has the option for the setting newaxis=None, in which case the value yielded by the generator is iterated over the "win" axis. In cases where the "win" axis is length 0, nothing is iterated (i.e., nothing published).

I had to make a change to the generator-level tests to drop yielded msgs with size 0, whereas previously I was dropping yielded msgs that were None.

The system-level tests are unchanged and continue to pass. If you have a use case that doesn't work with this PR then please create a test for that and I will try to figure out a solution.

@cboulay
Copy link
Collaborator Author

cboulay commented Mar 3, 2024

Oops. I did't meant to base on main. I'll let the tests complete then I'll rebase on dev.
Edit: Done.

@cboulay cboulay changed the base branch from main to dev March 3, 2024 16:46
@cboulay cboulay force-pushed the cboulay/win_gen_yield_type branch 2 times, most recently from d0057dd to d1535d2 Compare March 3, 2024 17:00
@cboulay
Copy link
Collaborator Author

cboulay commented Mar 14, 2024

@griffinmilsap , last time I changed windowing it broke one of your pipelines. Can you please check this one?

@griffinmilsap
Copy link
Collaborator

Window is such a beast. Easily one of our most complicated and heavily used additions to the signal processing library. I can say that depending on where this win dimension is placed, this will probably break some of my existing work, especially where (by happenstance) some of my pipelines actually already rely on a win dimension. How would you feel about changing win to something like _new_axis?

The idea of 0-length dimensions in AxisArray is also something I never really considered. It looks like the Unitization of this generator drops 0-length messages so this change won't result in higher messaging overhead for no reason, so that's good. In general, I'm in favor of retiring Optional data types because it makes linting easier and we don't have to check for is None later.

@pperanich do we rely on ezmsg.sigproc.window, or its generator implementation in any of the HLM or OI work? If not, given that this change only effects the generator, I'm for merging this if we can address the issue above.

As always, @cboulay thank you for the contributions; these are incredibly valuable and I apologize it took so long to get to this one.

@cboulay
Copy link
Collaborator Author

cboulay commented Apr 21, 2024

(rebased)

@griffinmilsap , I chose "win" because I thought that's what you were using but I thought it was scoped to exactly using a windowing node. I didn't think about what would happen if you applied the Window unit to an AxisArray that already had a "win" axis.

That is, if you tell the Window Unit that you don't want a newaxis and the generator inserts the "win" axis anyway for its own internal bookkeeping, it will be confused by the existing "win" axis.

Sorry about that.
I'm happy to rename the default bookkeeping axis to something else. How is "_windowing"? Ultimately it only matters for direct users of the generator, as in some offline pipelines.

Reminder to myself: Modify my offline pipelines to be explicit about the newaxis="win" argument.

@cboulay
Copy link
Collaborator Author

cboulay commented Apr 21, 2024

I accidentally had an unrelated commit sneak into this PR. It's similar in that it uses size-0 arrays in offline pipelines, but it's for a different processing step. I'll try to separate them. Edit: It's been removed and now exists in another commit (soon PR).

@griffinmilsap
Copy link
Collaborator

I think its time to merge this and if I run into issues on dev, I've got time to react to them :)

@griffinmilsap griffinmilsap merged commit 95e5326 into dev May 11, 2024
@cboulay cboulay deleted the cboulay/win_gen_yield_type branch May 12, 2024 05:00
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.

windowing generator should return an AxisArray not List[AxisArray]
2 participants