-
Notifications
You must be signed in to change notification settings - Fork 66
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
NDAttribute timeseries as circular buffer #388
Comments
I think the same approach we did with NDPluginStats probably makes sense. I’m happy for you to make a pull request. |
I have a dirty patch,... Current implementation of timeseries in NDAttribute performs callbacks per asyn port address, each containing a 1-D NDArray. If I want to keep this behavior I think I need a NDTimeSeries plugin instance for each NDAttributeN template instance. Also the NDArrays I allocate are single point, 1-D arrays. This is what I have right now. I like how NDStats does it with 1-D NDArray holding multiple value from the same time point. I think NDAttrabute could do the same for every attribute it tracks, but I'm not sure of the impact of such change on current users. My use case would favor this solution. @MarkRivers, what do you think? |
Pull request #389 contains changes to achieve behavior as described in the first paragraphs of the previous comment. |
I merged the pull request without reading your comment carefully enough. I agree that it should be changed to pass all of the values in a single NDArray, like the Stats plugin does. That way only a single time series plugin is plugin is needed I don't see how this would adversely impact current users, since it currently erases them all at the same time, they are not independent. I have made the change an pushed to Github. I have also
|
I see the difference, looks better than what I had. Also thanks for fixing missing bits in pull request! |
This issue is resolved by #389 |
Currently it is not possible to have NDAttribute plugin operate circular buffer of timeseries points. I guess this issue is similar to #327.
Would #333 be the way to go for this one, too? I can try to provide a patch.
The text was updated successfully, but these errors were encountered: