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

Add event lines to plots #18

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Add event lines to plots #18

merged 3 commits into from
Dec 13, 2024

Conversation

amitschang
Copy link
Member

@amitschang amitschang commented Dec 11, 2024

Adds lines to plotter for events.

Notes:

  • Unfortunately since the events most likely don't come from hardware, we need to send a separate request to get them (the sensor request specifies name). Only alternatives I think would be to either treat events separately in the history endpoint (not sure I like that) or just retrieve all data.
  • I noticed the shape of data might not be ideal, iso {'name': [{...}], 'name2': [{...}]}, might want to change backend to [{'name': 'name', ...}, {'name': 'name2', ...}]. This would replace the awkwardness of the flatmap currently done on events, and also preserve the name in each record. can do in separate change with backend
  • The time axis was categorical, so lines not exactly corresponding to a point would not render. It has changed to numeric and with a formatter to render as time.
  • I couldn't figure out how to make labels vertical or better-yet show up as a tooltip, so any useful log message ends up cluttering and possibly unreadable...

Example SS:

image

@amitschang amitschang marked this pull request as ready for review December 12, 2024 16:02
@amitschang amitschang requested a review from imaitland December 12, 2024 16:02
@amitschang amitschang changed the title event lines wip Add event lines to plots Dec 12, 2024
client: evolverClient,
});

const events = await Evolver.history({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Promise.all to parralleize these two request.

const [historyData, eventsData] = Promise.allSettled([await Evolver.history({whatever}), await Evolver.history({whatever + kinds["event"]})])

<ReferenceLine
key={event}
x={event.timestamp}
label={event.data.message}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Label will take a component as prop, and daisyUI provides a tooltip: https://daisyui.com/components/tooltip/,

label = {() => {
  return (
    <div className="tooltip" data-tip={event.data.message}>
      {event.data.message.length > 20 ? `${event.data.message.slice(0, 20)}...` : event.data.message}
    </div>
  )
}}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks cool, but when I do this I don't see any text at all. I tried with plain div and span elements like this, doesn't seem to show. There may be some requirements it has on the underlying element...

// array of events. Here we drop name, probably change to backend could keep
// the name in the struct
const allEvents = flatMap(events);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, did you test this change with a chart that shows multiple vials' histories?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the thing worked around here is that event will be tagged from a particular logger (main device, or controller), e.g.

{ "main_device": [...], "controller": [...]}

each may be tagged with vial (shown only for that plot) or not (global), but we need to bring all those named things into a single array. I think I might change backend shape since either you wan't it all where each element says where it came from, or you are selecting only one, where this structure is redundant 😄

@@ -57,6 +81,14 @@ export const HardwareLineChart = ({ vials, rawData, property = "raw" }) => {
connectNulls={true}
/>
))}
{eventData.map((event) => (
<ReferenceLine
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@amitschang
Copy link
Member Author

Made updates and re-requesting review. To do the label thing well looks to require more effort, so I think what we can do is merge this in (if otherwise good) and make label change as an enhancement (it affects only a single line in this PR)

Copy link
Collaborator

@imaitland imaitland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@amitschang
Copy link
Member Author

thanks!

@amitschang amitschang merged commit 960acc6 into main Dec 13, 2024
3 checks passed
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.

2 participants