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 radar graph feature #1039

Merged

Conversation

nathang15
Copy link
Contributor

@nathang15 nathang15 commented Oct 11, 2023

Description

Finish up the work on Radar Graph. These includes fixing the display, add date range picker, allow groups to work on radar.

The code for Date Range Picker is taken from 3D graph branch of @ChrisMart21.

As the commits show, @Elias0127 and @JesseVarGar did the initial work on this feature and @nathang15 finished it up.

Fixes #208

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

JesseVarGar and others added 30 commits August 1, 2023 18:33
@nathang15
Copy link
Contributor Author

This is a linting error. Weird thing is that my local device does not show any errors regarding linting.

@huss
Copy link
Member

huss commented Oct 12, 2023

I will try to look at this soon. As for the linting errors, if you open a shell on the OED web container and run "npm run check" you should see the same linting errors. It looks as if your VSC is using spaces instead of tabs for indenting. You can change that by clicking the indentation on the bottom ribbon in VSC. There you can set to use tabs and can convert the current spaces to tabs. Let me know if this does not work.

@nathang15
Copy link
Contributor Author

I believe the indentation is now fixed.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

Thanks to @nathang15 for this PR. I need to review the code but wanted to comment on a few things I noticed when initially trying it. Overall it looks good.

  1. The line and radar Redux state is very similar (line also has max/min). Given the current plan is to use the line values for radar, I don't think we need the radar state. This is what is done for maps where it uses the bar state. Unlike that case, you do need to potentially fetch the new date ranges selected if not already in Redux state.
  2. The time set from the date range picker for the final day is 23:59:59. This can be seen from the Redux state. This potentially causes the last point in the day to be dropped. For example, if you select 1 day for 15 min test data you get 95 instead of 96 points. The last point is at 11:37:30 instead of 11:52:30. OED uses the end of a day as 00:00:00 of the next day. This can be seen in the bar state/values on the graph.
  3. The dates seem off when multiple meters are graphed with different date/time values. This figure shows the 15 min and 4 day sin where the dates are repeated. I suspect the label algorithm chooses different labels in these two cases. Somehow they needs to be the same. I think other cases also show issues.

Screenshot 2023-10-14 094157

nathang15 and others added 19 commits October 18, 2023 00:39
This includes both the solution and a lot of commented out or extra code
tha was attempts to get different plotly features to work. Done to show
what tried and will remove soon. In the end this was not allowed.
TODO notes some changes expected when 3D is merged with this.
As noted, it is hoped that a direct alter will work in migration.
- Fix problem where lines with different number of points caused issue.
- Clean up code.
Thiis is consistent with other menus and desireable now that
more items on menu.
Mostly comments but also remove redundant variable.
- Note may not need radar state & remove unneeded action type.
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

Thanks to @JesseVarGar & @Elias0127 for the initial work on radar and @nathang15 for additional work. I have review the code, made a few changes and found that it is fine and works as expected. This should be ready to merge.

@huss huss merged commit 85e9053 into OpenEnergyDashboard:development Dec 3, 2023
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.

radar/clock graph
4 participants