-
Notifications
You must be signed in to change notification settings - Fork 77
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
ENH/DEV: Formula Viewer for Archive Viewer #1095
Conversation
9b4365c
to
6617140
Compare
a3e8d88
to
d41f1b1
Compare
d41f1b1
to
8ce5ecd
Compare
Hi there @YektaY @nstelter-slac @jbellister-slac I was wondering if one or more of you would be free to look at this when you get the chance? Hoping to finish this because I have a couple of other branches that are dependent on this. Thanks so much in advance! |
I'm not one of the at-mentions, but I'm a bit confused by this PR because the documentation is sparse. What's the expected usage here? Why is this implemented strictly for the archive viewer, rather than being implemented more broadly like the calc plugin is? What kinds of new/better/easier displays can we make with this PR? I think having an explanation in the PR description and/or an update to the PyDM documentation pages related to this feature as part of the PR would go a long way in making this reviewable. |
Hi @ZLLentz, yes sorry the documentation is sparse. I'll add to it soon, but I thought I'd respond first.
One of the requested functionalities on the archive viewer was the ability to graph functions of PVs as opposed to just the PVs themselves. Currently, my project is focused on enhancing the archive viewer, so I made changes under that context. I am happy to broaden that context to support usage outside of the archive viewer - would you have recommendations/suggestions for what you would like that to look like?. The FormulaCurveItem class extends a BasePlotCurveItem, which means tools that use baseplot will also be able to use the formulas. Currently we expect archived data, so one fix would be removing that expectation such that formulas could be used with timeplots as well.
If other projects would ever need the ability to graph functions of PVs, this class could be useful for them. |
33cc795
to
9147374
Compare
Ok, thank you very much. I think I am ready for review fully now @nstelter-slac |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice patch! sry for the delay
added some suggestions I had, mainly I think there is some stuff that could make the main logic part a bit more readable and therefore maintainable.
lets try to get at least one other reviewer on this before we merge
thanks!
23da80d
to
50b5977
Compare
50b5977
to
9a4a723
Compare
e4fd72a
to
14b8e59
Compare
14b8e59
to
1dce653
Compare
8adaa27
to
74d308f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the additional context to the PR description, and thanks also for your patience while we take a look at this. It is looking very good!
My main thought on the first pass through is that there's quite a lot of data processing happening in a GUI widget here - I think most of it could be potentially moved to the data plugin layer.
One possible way of putting this together would be to use the calc plugin prefix. An end user can then enter in a formula like here, this example only uses a single PV but multiple PVs are fine too:
pydm/examples/calc/sin_cos_tan.ui
Line 60 in 8b27429
<string>{"y_channel": "calc://circley?angle=ca://DEMO:ANGLE&expr=math.sin(math.radians(180-angle))", "x_channel": "calc://circleX?angle=ca://DEMO:ANGLE&expr=-1*math.cos(math.radians(180-angle))", "name": "Angle", "color": "#ffd509", "lineStyle": 0, "lineWidth": 5, "symbol": "o", "symbolSize": 5, "redraw_mode": 3, "buffer_size": 1}</string> |
The archiver_plugin
would need to be modified to handle this case (using the code you've already written). Then the archiver_time_plot
becomes simpler after this as it can just say give me the result of this calculation between these PVs and display it. It no longer has to maintain its own list/dict of curve items for its calculations, that's all handled for it now. The live data functionality is also free at that point since it's just already what the time plot does using the calc plugin, and maybe the new FormulaCurveItem
can just inherit most of its functionality from ArchivePlotCurveItem
at that point. I think this could be cleaner and easier to maintain. And makes it easier if other widgets or clients want the same functionality.
That said, it might be easiest to get some of these PRs merged in before any potential refactor so the relevant code is all moved at once. And I might still need to get some more context to fully understand the precise use case that we're going for here since it looks like it involves the archiver application built on top of PyDM. But the code looks great so far and works well! This will be nice functionality to add.
Also if it would be helpful, I'm happy to drop by early next week to chat in person about any of this, or on zoom would be fine too. |
…ide as opposed to pydm side (PR Suggestion)
Texting you on Slack about meeting next week for this! Thank you |
76ed36a
to
e3471c1
Compare
e3471c1
to
8cedfda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me
Created class FormulaCurveItem. This accepts a formula (in the form of a string) and a dictionary of string : BasePlotCurveItem and populates a new data buffer with the function of the data being passed in. Can be used to graph functions of other functions, by passing FormulaCurveItems in the dictionary and referencing them in the formula string.
This is useful for the Archive Viewer because one of the requested functionalities is the ability to graph a function of one or more PVs rather than graphing only PVs themselves. As the new class extends BasePlotCurveItem, this is usable in other contexts where BasePlotCurveItems are being used.