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

Bug: sigproc.Counter time offset is over-adjusted #59

Closed cboulay closed 4 months ago

cboulay commented 7 months ago

If I understand TimeAxis offset correctly, it provides the time of the first sample in an AxisArray.

Thus, in the following block,

https://github.com/iscoe/ezmsg/blob/43fef1835e6a7fadba65a65e9b6dba83a30d4245/extensions/ezmsg-sigproc/ezmsg/sigproc/synth.py#L130-L140

it should read

offset_adj = (self.STATE.cur_settings.n_time - 1) / self.STATE.cur_settings.fs

e.g., if there is only 1 sample then the adjustment should be 0.

cboulay commented 7 months ago

I'm working on unit tests for sigproc.synth.Counter and fixing its timing.

@griffinmilsap , how should the time axis offsets be generated when dispatch_rate is a float? Consider the following scenarios:

settings1 = CounterSettings(n_time=10, fs=100.0, dispatch_rate=10.0)
settings2 = CounterSettings(n_time=10, fs=100.0, dispatch_rate=2.0)
settings3 = CounterSettings(n_time=10, fs=100.0, dispatch_rate=20.0)

In each case, we have 10 samples/chunk.

In settings1, the target dispatch rate is 10.0 chunks/s so we get 10.0 * 10 = 100 samples/s, which is exactly the fs. This one is easy.

In settings2, the fs is still 100.0 but we're only pushing 2 chunks / s or 20 Hz-worth of samples. Should each chunk's time-axis' .offset increment by 0.5 to be consistent with the real time or should they increment by 0.1 to be consistent with the provided fs? We can't have both.

Similarly in settings3, does .offset increment by 0.05 or by 0.1?

My opinion is that providing a dispatch_rate of anything other than "realtime" should decouple the Counter generator from any concept of wall clock, meaning the system can run as though it's operating faster or slower than realtime. In this (!= "realtime") case, the .offsets should be entirely synthetic, not from time.time(), and they should be generated so that the number of samples per synthetic-second is consistent with the provided fs.

Does that sound good?

Follow-up: If yes, then what .offset should be associated with samp == 0, i.e. off_0? Should it be 0.0 or should it be the time.time() of the first chunk? If the latter, do we reset it when samp wraps around mod or do we continue incrementing from the original? See options below.

Option off_0 begin off_0 wrap
A 0.0 0.0
B time.time() time.time()
C time.time() t_zero + fs * mod

I think B isn't a real option because it causes the .offsets and the number of samples to be inconsistent with .fs. Taken another way: do we want to use time.time() as an anchor for the zero'th offset even if the subsequent offsets have nothing to do with time.time()?

griffinmilsap commented 7 months ago

I'm 100% on board with your suggestion here, and I've accepted your PR already. Completely decoupling from wall-clock time is the correct action. How about we introduce a new setting:

t_zero: Optional[float] = None

If none, and dispatch rate is realtime, it uses time.time() to define t_zero and behaves like normal. If None, and dispatch rate is some float, it defaults to 0.0 and behaves from there onward with behavior C, which I like because offset continues to increase as it should.

cboulay commented 6 months ago

Just a reminder to myself that I tackled essentially this same problem recently when I added playback-over-LSL to pyxdf. https://github.com/xdf-modules/pyxdf/pull/95/commits/923fce2dce00216fcb21ebef68bca00502f79b36

I should refer to that (tested) solution when coming back to this issue.