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

Block times type is too restrictive #34

Open
jpsamaroo opened this issue Feb 27, 2022 · 1 comment · May be fixed by #35
Open

Block times type is too restrictive #34

jpsamaroo opened this issue Feb 27, 2022 · 1 comment · May be fixed by #35

Comments

@jpsamaroo
Copy link

jpsamaroo commented Feb 27, 2022

I'm trying to setup integration with PortAudio.jl so that I can sample from my computer's microphone, which samples on the order of tens of microseconds. I've tried constructing a Block with the times calculated based on the number of returns audio frames, however due to rounding (because DateTime only has Millisecond resolution), I receive an error in the Block constructor that my times are not strictly increasing.

As a workaround, I've defined a PreciseDateTime struct which contains a DateTime and Nanosecond, to provide greater specificity; unfortunately, Block is too restrictive and does not allow passing times with an eltype other than DateTime.

Would it be reasonable to relax this restriction and allow any <:AbstractDateTime instead? It would probably add some amount of complexity when mixing Blocks from multiple sources (as we then need to promote to the more precise time type), but that feels like an inevitable tradeoff when wanting to support finer time resolutions.

@jpsamaroo jpsamaroo linked a pull request Feb 27, 2022 that will close this issue
@tpgillam
Copy link
Owner

tpgillam commented Feb 28, 2022

I certainly agree that there's no reason that the "Time" type used in TimeDag has to be a DateTime, and it's been at the back of my mind as a generalisation [1] that may be wanted at some point. It wasn't immediately required for our use-case, but good to know that there is interest in having this feature!

Is a generalisation of Block sufficient for your needs? I'm not completely against [3] doing something like this in isolation if it is useful for you. However, on its own it wouldn't work nicely with the rest of TimeDag, and calling block_node on it may lead to unhappiness at some point when evaluating the graph [2].

Generalising the time type everywhere is certainly possible, but is probably not a small task (it requires adding type parameters in many places — at least to Node, and NodeOp & all descendants — and we may need to specify a TimeDelta parameter in addition to just Time)

complexity when mixing Blocks from multiple sources

Yep this is a concern. My gut reaction is to simply not support combining nodes with different time types (since this could indicate a more fundamental incompatibility — especially if permitting time-types beyond AbstractDateTime), but maybe we could.


[1] In fact, I wouldn't stop at AbstractDateTime :) Any type that implements basic comparison & addition should be usable — e.g. using a plain Int64 should be possible, although there's a few API details that need to be considered. When specifying a timedelta window to functions like mean, currently this will take any TimePeriod and convert to Milliseconds for internal use. We'd probably need a little wrapper type to avoid confusion with this method for the same function, and similarly elsewhere.

[2] Internally, each run_node! implementation will create a new Block that uses DateTime times. So, you'd lose time precision when evaluating, and there could be worse consequences as a result (due to the fact that, after precision loss, you may end up with duplicated timestamps — these aren't permitted).

[3] It's a step in the right direction — but my concern is that it could lead to confusion, since they wouldn't be supported by the rest of this package.

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 a pull request may close this issue.

2 participants