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

Velocity/Speed histogram in drive details dashboard #3836

Conversation

jheredianet
Copy link
Contributor

@jheredianet jheredianet commented Apr 12, 2024

Here is the adaptation to the change that has been suggested here #3835

Please, I would like, before doing a merge, for someone to manually import this dashboard into their test environment to see if any information or improvements are missing.

image

Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit f1afdf3
🔍 Latest deploy log https://app.netlify.com/sites/teslamate/deploys/661b10476a60720008c5d55b
😎 Deploy Preview https://deploy-preview-3836--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Apr 12, 2024

I test with our ci generated image per PR, in this case:

grafana:
       # image: teslamate/grafana:latest
      image: ghcr.io/teslamate-org/teslamate/grafana:pr-3836

@JakobLichterfeld JakobLichterfeld added the enhancement New feature or request label Apr 12, 2024
@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Apr 12, 2024

Thank you!

Looks nearly perfect! Love the additional statistics.
image
image

Minor changes:

  • I would prefer if the Speed Histogram goes from 0 from the left to max on the right
  • I would remove the unit on every x axis point
  • I would add percentage above the bars (or both, Minutes and percentage)

@jheredianet
Copy link
Contributor Author

Minor changes:

  • I would prefer if the Speed Histogram goes from 0 from the left to max on the right
  • I would remove the unit on every x axis point
  • I would add percentage above the bars (or both, Minutes and percentage)

Ok, I will do the updates suggested. Need a little extra time.

@cwanja cwanja linked an issue Apr 12, 2024 that may be closed by this pull request
@jheredianet
Copy link
Contributor Author

Minor changes:

  • I would prefer if the Speed Histogram goes from 0 from the left to max on the right
  • I would remove the unit on every x axis point
  • I would add percentage above the bars (or both, Minutes and percentage)

@JakobLichterfeld What about now?

image

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Apr 13, 2024

Nearly perfect.
image

If there are lots of different speeds, the x-axis got a bit messy (see below), therefore

  • I would remove the unit on every x axis point

image

Additionally, the color shaded are too narrow in that case. I personally preferred the coloring from the first version.

@jheredianet
Copy link
Contributor Author

Remove the unit on every x axis point

What about now? I hope this one to be the perfect one ;-)

image

@sdwalker
Copy link
Contributor

Missing units for the speed (an existing issue) but looks good otherwise
mi
image
km
image

Standardize the letter casing for all of the text?
Battery heater -> mixed casing
front left -> lower casing
Outside Temperature -> upper casing

@jheredianet
Copy link
Contributor Author

jheredianet commented Apr 13, 2024

Standardize the letter casing

HI, I took the opportunity to consider your suggestions as well using upper casing ;-)

@JakobLichterfeld
Copy link
Collaborator

Awesome, ty!
image

@JakobLichterfeld JakobLichterfeld merged commit f4fb311 into teslamate-org:master Apr 14, 2024
12 checks passed
@jheredianet jheredianet deleted the Add-Speeds-to-Drive-Details-dashboard branch April 15, 2024 06:48
@tjhart
Copy link

tjhart commented Apr 16, 2024

@jheredianet

As I looked at this PR with regard to #3763, I noticed that the speed scale in the Drive panel now extends into the negative range. Is that realistic? Would that data set ever extend below zero? If not, why are we limiting that data to half of the vertical space in the graph? Are there other data sets in that graph that are now constrained to the top half of the graph with this change?

@cwanja
Copy link
Collaborator

cwanja commented Apr 16, 2024

@JakobLichterfeld (or anyone for that matter) what is the syntax to update your YML to pull a specific PR? I forget.

Is it:image: teslamate/teslamate:pr-####? This would be image: teslamate/teslamate:pr-3836.

@jheredianet
Copy link
Contributor Author

@jheredianet

As I looked at this PR with regard to #3763, I noticed that the speed scale in the Drive panel now extends into the negative range. Is that realistic? Would that data set ever extend below zero? If not, why are we limiting that data to half of the vertical space in the graph? Are there other data sets in that graph that are now constrained to the top half of the graph with this change?

Yes, as I've explained before, it's not only the speed that is represented in that axis. There are many metrics there. So, the challenge is if we adapt the axis for a specific metric, it would not be well represented for other one, like the Power which is the one that has positive and negative values (and even elevation perhaps). Maybe the Solomonic solution could be separating the familiar metrics in another graph (just as an idea)

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Apr 17, 2024

what is the syntax to update your YML to pull a specific PR? I forget.

grafana:
       # image: teslamate/grafana:latest
      image: ghcr.io/teslamate-org/teslamate/grafana:pr-3836

similar for TeslaMate image, will update the contributing doc about this

@JakobLichterfeld
Copy link
Collaborator

JakobLichterfeld commented Apr 17, 2024

will update the contributing doc about this

done,( #3856 ), see https://docs.teslamate.org/docs/development/#testing-with-our-ci-which-builds-the-docker-images-automatically-per-pr

@cwanja
Copy link
Collaborator

cwanja commented Apr 18, 2024

Hate to say it, but with this PR the drive graph is completely useless for range in my opinion. The lines are all on top of each other. Beyond speed and power, the lines mean nothing. I think we should roll this back @JakobLichterfeld @jheredianet.

This is the exact same drive from the screenshots in this comment for comparison. #3763 (comment)
Screenshot 2024-04-17 at 8 27 58 PM

@jheredianet
Copy link
Contributor Author

Hate to say it, but with this PR the drive graph is completely useless for range in my opinion. The lines are all on top of each other. Beyond speed and power, the lines mean nothing. I think we should roll this back @JakobLichterfeld @jheredianet.

We return to the same thing that has already been mentioned before (at #3763 (comment)), in this panel the Y axis shows different units (speed per hour, power, range) and that is why it cannot fit for everything. From my point of view, I don't see a major problem about the ranges, beyond the fact that they are seen above the speed, the important thing for me is that both range lines show that there are no significant changes between them.

I don't think it's necessary to revert the PR, since there are other changes outside of this panel, on the contrary, I think we can try to improve it together.

Maybe you could try to change is the "center cero" axis in this panel and then modify the Overwrite "Add Standard options > Min 0" you suggested to see if you get what you want. If so, a PR with those changes will be enough.

@JakobLichterfeld
Copy link
Collaborator

Hate to say it, but with this PR the drive graph is completely useless for range in my opinion.

A reduce in range of 10 miles is so little, the graph is ok from my point of view.

@DrMichael
Copy link
Collaborator

DrMichael commented Apr 18, 2024

Actually, I agree with @cwanja. But I think, the ranges and battery heater are not really relevant for a drive. Spee, SOC and Usable SOC might be the essential informations...
With ranges, batter heater:
image
Without (and centred_zero off):
image
Actually, turning off only centered zero makes it also a bit more comprehensible?
image

@jheredianet
Copy link
Contributor Author

.... I think, the ranges and battery heater are not really relevant for a drive. Spee, SOC and Usable SOC might be the essential informations...

I agree with @DrMichael for a drive is only important Speed, SOC, Power (and even elevation as a right Y axis) as I did in this dashboard https://github.com/jheredianet/Teslamate-CustomGrafanaDashboards?tab=readme-ov-file#tracking-drives

image

That's why in a previous comment I've suggested as an idea, that maybe the Solomonic solution could be separating the familiar metrics in another graph (meaning Ranges and other metrics that do not interfere with units)

@DrMichael
Copy link
Collaborator

DrMichael commented Apr 18, 2024

@jheredianet What is the rationale to have centered zero set to on?

@cwanja
Copy link
Collaborator

cwanja commented Apr 18, 2024

Hate to say it, but with this PR the drive graph is completely useless for range in my opinion.

A reduce in range of 10 miles is so little, the graph is ok from my point of view.

@JakobLichterfeld I would agree that 10 miles is little range loss, however when comparing old and new graphs for the exact same drive it got worse from a readability standpoint. No more or less data is present on the new graph, it's just less readable and usable in my opinion.

@cwanja
Copy link
Collaborator

cwanja commented Apr 18, 2024

Actually, turning off only centered zero makes it also a bit more comprehensible?

I'd align with @DrMichael's third graph here. Would love to test it against the same drive.

@jheredianet
Copy link
Contributor Author

@jheredianet What is the rationale to have centered zero set to on?

It's because of the "Power" since it could be posistive and negative

@DrMichael
Copy link
Collaborator

But you don't need centered zero for that...

@jheredianet
Copy link
Contributor Author

But you don't need centered zero for that...

You're right, but having in mind that in this case I'm showing the elevation

With no "centered cero", the elevation seems that I'm traveling across a mountain (when in reality there is only a small variation around 100m)
image

With "centered cero" it looks better and elevation seems to be almost as an horizontal line (more accurate in reality and it is in tune with the power):
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dashboard Related to a Grafana dashboard enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat request: velocity histogram in drive dashboard
6 participants