dbuezas / lovelace-plotly-graph-card

Highly customisable Lovelace card to plot interactive graphs. Brings scrolling, zooming, and much more!
428 stars 20 forks source link

Feature request: add offset field #33

Closed zanna-37 closed 2 years ago

zanna-37 commented 2 years ago

https://github.com/RomRider/apexcharts-card has a offset field that is used to "move" the x values of a series. I already tried to use data transformation, but the problem is that values outside the graph are not available.

Why is it a problem? I have an entity that gives a forecast of a value 12h in the future. I therefore need to move the entity values 12h in the future because the current value refers to a future event. However, I can only "move" data that are already visible, which leaves a blank space. If I scroll left, the data is loaded, but it is of course inconvenient.

immagine


The transformation I used:

# this works only partially
    lambda: |-
      (ys, xs) => {
        const result = {x:[], y:[]}
        for (let i = 0; i < xs.length; i++) {
          const x = xs[i].setHours(xs[i].getHours()+12) # move the x-values (<--)
          const y = ys[i]
          if (x<((new Date).setHours((new Date).getHours()+2))){ # only show 2 hours in the furure (not relevant for this issue)
            result.y.push(y)
            result.x.push(x)
          }
        }
        return result;
      }
zanna-37 commented 2 years ago

Is there anything that can be done?

dbuezas commented 2 years ago

Hi zanna, Please send me how the apex yaml with offset looks like an I'll add the feature

zanna-37 commented 2 years ago

There are 3 relevant rows.

type: custom:apexcharts-card
graph_span: 12h
span:
  offset: +2h  # <------------------- HERE and...
               # This indicates how much of the future the graph should show in the initial view
series:
  # This serie outputs the forecasted power production in 12 hours from now
  - entity: sensor.power_forecast_production_in_12hours
    group_by:
      func: raw
    offset: '-12h'  # <------------------- ...HERE and...
                    # This shifts the serie
    extend_to: false
    float_precision: 0
    color: '#AAAAAA'
    stroke_width: 1
    show:
      hidden_by_default: false
    yaxis_id: watt
  # This serie outputs the forecasted power production for the next hour
  - entity: sensor.power_forecast_production_next_hour
    group_by:
      func: raw
    offset: '-1h'  # <------------------- ...HERE
                    # This shifts the serie
    extend_to: false
    float_precision: 0
    color: '#333333'
    stroke_width: 1
    show:
      hidden_by_default: false
    yaxis_id: watt
yaxis:
  - id: watt
    decimals: 0
    apex_config:
      forceNiceScale: true

Side note: I argue that the '-1h' should instead be '+1h'. Because it is shifting a value that happens now, to the future. The way apex implemented it is confusing but that's a minor detail.

zanna-37 commented 2 years ago

If you need more assistance or if you want me to describe the feature better, just ask. Thank you!

dbuezas commented 2 years ago

Thanks, I haven't sat down on this yet. I think it will be fairly to implement. I'm not sure I understand what the span: offset: 2h does. Can you expand on that?

zanna-37 commented 2 years ago

It basically tells how much "future" we see by default. (See the purple annotation)

image


More context: The real grey line (before applying the +12h offset) would have been much more on the left. What you see here is the one that has been shifted in the future by 12h.

dbuezas commented 2 years ago

Oh, I see. Let's review the algorithm I have in mind:

  1. Get the range visible in the screen
  2. Shift the visible rangy by span: offset: Expand its upper limit it by span:offset:
  3. Shift the range by the entity's offset
  4. Fetch the resulting range
  5. Subtract the entity offset back from each data point returned by HA
  6. Plot

Did I get it right?

dbuezas commented 2 years ago

Mmmm. No.

  1. Should be: shift the visible rangy by span: offset:
zanna-37 commented 2 years ago

Let's assume that:

Then:

dbuezas commented 2 years ago

And that's what you expect, right?

zanna-37 commented 2 years ago

Yes, I just augmented your algorithm with an example, so there is less ambiguity.

dbuezas commented 2 years ago

Perfect, i may try it today

dbuezas commented 2 years ago

I just gave it a try and it seems to try. I'll try to make a veta release tomorrow and I'd really appreciate it if you tested it a bit more (I don't know the use case that well) :)

Also, do you want to write down the lines for the readme later? It can be done in github's UI

zanna-37 commented 2 years ago

Sure, of course. =)

dbuezas commented 2 years ago

This got really tricky, there is some bug in plotly (maybe) that makes this feature jump like crazy. I think it is because there is data out of the visible range right from the start.

Anyway, I think I found how to fix that, and it will make the non-offsetted plots less buggy too!

I'll push a beta version for you to try and give feedback on, I'd appreciate it if you tell me any bugs you find and also PR the instructions (maybe with some pics? :) ).

If everything goes well this is almost done, just needs some cleanup.

dbuezas commented 2 years ago

https://github.com/dbuezas/lovelace-plotly-graph-card/releases/tag/v1.6.0-alpha This feature owes me 5 hours of my life

zanna-37 commented 2 years ago

Amazing! Thank you very much! If you ever need something on my repo you'll have priority support. 😉 By the way, I think we are getting there but there are still issues.

When I click the drag button this message is shown: "Error computing range subtraction. Please report an issue in the repo of this card and share this:[null,null][949522275000,981144675000]"

I'll have a look into it myself.

zanna-37 commented 2 years ago

Ok, I'm working on it and I'll post a pull request once I finish.

One question though, why did you decide to remove this piece of code https://github.com/dbuezas/lovelace-plotly-graph-card/commit/cc7aed03f101ccd13dbc8ed84a0bc1391b422bf4#diff-f0b1423191726c0f2981ced42942c5c7743094bb86c9c05495ca9fa00a855583L43-L63?

dbuezas commented 2 years ago

Oh, that's awesome that you'll make a PR! I have an unpushed commit with a bunch of fixes, lemme push that it first (will save you some headaches)

The removal of that code was because it was getting in my way, I'll put it back

dbuezas commented 2 years ago

There is one bug in ploty which is making me crazy: if there are discontinuities in the plot (unavailable, which is transformed to null to let plotly know to leave a gap), there are a lot of artifacts when scrolling

dbuezas commented 2 years ago

Ill push in an hour (not at my computer now)

dbuezas commented 2 years ago

The removed piece of code may the culprit of this : https://github.com/dbuezas/lovelace-plotly-graph-card/discussions/92#discussioncomment-4026237

That piece of code was mostly there to force points in the presebt, and have the visible range be correct by letting plotly autoscale the xaxis. With the offset feature, this trick is no longer viable so I implemented it the right way. But now I have this ghost traces. You'll see in the upcoming commit

dbuezas commented 2 years ago

And your component looks super useful! I'll definitely give it a try :)

zanna-37 commented 2 years ago

In the removed piece of code, can you reword the documentation that you wrote so it is clearer? Maybe with some examples. I would like to fix that as well, but I need to deeply understand it first.

dbuezas commented 2 years ago

Ok, I hadn't changed that much, just a fix for the NaN in ranges. The branch is now up to date, go for it! :)

Regarding the removed piece of code, it may stay gone because of this:

The removed piece of code may the culprit of this : https://github.com/dbuezas/lovelace-plotly-graph-card/discussions/92#discussioncomment-4026237

That piece of code was mostly there to force points in the presebt, and have the visible range be correct by letting plotly autoscale the xaxis. With the offset feature, this trick is no longer viable so I implemented it the right way. But now I have this ghost traces. You'll see in the upcoming commit

dbuezas commented 2 years ago

Regarding the docs, it was too long ago... I'm not sure anymore :(

dbuezas commented 2 years ago

I manage to reproduce the misplotting I got, and find the plotly bug:

type: custom:plotly-graph-dev
entities:
  - entity: sensor.nothin
    lambda: |-
      () => ({
        x:[-60,-50,-40   ,-30     ,-20 ,-10, 0].map(x => new Date(Date.now()+x*1000*60)),
        y:[0. ,1. ,null ,3       ,null,4,5],
      })
hours_to_show: 1

it happens in the weird case where there is a single value sandwiched by 2 nulls. In this case, plotly seems to lose track of its traces (I hypothesise this is because the single number is not a line, so some for loop probably stops early).

The offset feature forced me to implement correctly some hacks that did bother me, and produced some odd jumps here and there. Interestingly (and annoyingly) they also hid this plotly bug.

zanna-37 commented 2 years ago

Ok, so what do you propose?

dbuezas commented 2 years ago

Did you fix the bug you found? Or did my push fix it? I'll fix this small bug i found, but the rest is more or less final. You can already make a pr with that branch with the readme docs :) you have some real use case and understanding. Btw, are the signs of the offset intuitive to you like they are?

dbuezas commented 2 years ago

Ok, so what do you propose?

I guess you are asking what my proposed solution is. I was thinking about duplicating any lonely data point that finds itself between unavailables. That should give plotly some rest. I also need to confirm the deleted block you mentioned really isn't needed.

zanna-37 commented 2 years ago

The deleted block is needed to delete the fictitious data points that HA make up. I'm working on it.

I was thinking about duplicating any lonely data point that finds itself between unavailables. That should give plotly some rest.

About this, I do not experience the problem that you are pointing out. I think it should live in a separated issue.

dbuezas commented 2 years ago

I think it should live in a separated issue.

You can reproduce it with the lambda. It's true that it sounds unrelated to the offsets, but the old code hid this effect (i assume because the range of the xaxis was never set explicitly)

dbuezas commented 2 years ago

Did you fix the bug you found before?

zanna-37 commented 2 years ago

Did you fix the bug you found before?

Nope, the problem is caused by the fictitious data points that HA adds. I need I way to detect them and I'm working on that. Once it's fixed I think this issue is solved.

You can reproduce it with the lambda. It's true that it sounds unrelated to the offsets, but the old code hid this effect (i assume because the range of the xaxis was never set explicitly)

Now I see. Anyway let's make a separate issue. Before merging everything on master we solve both of them.

dbuezas commented 2 years ago

I need I way to detect them

Idea: They may have exact same timestamp as the end date of the fetched data. In that case, one option may be to always fetch up to end timestamp + 1 and then always remove the last one

dbuezas commented 2 years ago

Ghosting on lonely datapoints sandwiched between unavailable is fixed in this commit to the offsets branch.

Update: I don't know how I got confused, but the but the bug is in master too, so this is (as you said) totally unrelated to this issue

zanna-37 commented 2 years ago

Ghosting on lonely datapoints sandwiched between unavailable is fixed in ~this commit to the offsets branch~.

Update: I don't know how I got confused, but the but the bug is in master too, so this is (as you said) totally unrelated to this issue

Oh ok, wouldn't it be appropriate to open a issue on plotly?

Idea: They may have exact same timestamp as the end date of the fetched data. In that case, one option may be to always fetch up to end timestamp + 1 and then always remove the last one

This might work however:

dbuezas commented 2 years ago

a data point might coincide with the range boundary

Yes, but we can fetch up to end+1. If a data point coincides, we didn't want it anyway.

dbuezas commented 2 years ago

Oh ok, wouldn't it be appropriate to open a issue on plotly?

True, I'll do that. I don't have high hopes though, as they tend to ask for a "$PONSOR" when they get corner case bugs (not judging, the lib is a behemoth)

zanna-37 commented 2 years ago

I'm close to a working solution, but I have work next days, so I'll do the best I can. Don't worry, I'll keep you posted. 😉

In the meantime, since the branch has gone a little crazy with all the commits, do you mind if I rewrite the commit history (squashing and moving commits) so it's clearer? I'll do the history-rewrite in my fork, and I'll then create the pull request. If you are against it, and prefer to leave the commits as is, just tell me.

dbuezas commented 2 years ago

As long as it doesn't change the line authors, I'm fine with that :). In any case I'll squash and merge the pr branch to master in the end, so it shouldn't make a diff I have to start working myself in the next couple of days, and I really want to merge the other autorange changes to master before that (master is a bit bugy at the moment).

zanna-37 commented 2 years ago

Ok, can you give me a deadline? If we can't make it, we should just extract (read cherrypick) the autorange changes from the branch and apply them on master.

dbuezas commented 2 years ago

Tomorrow night? :) Then I can do it on Friday Btw: #104 this may mean that the old fix is doing more harm than good

zanna-37 commented 2 years ago

By "old fix" you mean the removed code right?

dbuezas commented 2 years ago

By "old fix" you mean the removed code right?

Yes

dbuezas commented 2 years ago

And for reference, #105 is one of the fixes in this branch. I thought it was specific to the offsets branch, but it isn't.

zanna-37 commented 2 years ago

For the sake of having things compartmentalized, I suggest leaving this issue just for the offset. I'll take care of implementing it myself based on what you already did.

Everything else (that lives on the offset-branch) should be cherry-picked and pushed to master separately. In this way, you can push the important fixes fast, and I have the time to review the rest.


To sum up:


A note on the "removed code". I'm not sure if it is better to temporarily remove it from master until I fix it, or leave it there. Your choice. (If you want my 2 cents, I'd leave it there)

dbuezas commented 2 years ago

The risk if I sit down and cherry pick, rebase, etc, is that I may end up also accidentally fixing the "removed code" part :)

Afaik, the offset code already works fine, it is only the removal of duplicate points that I ignored. Extending to Date.now is not needed anymore now that the range is set explicitly to match hours_to_show

zanna-37 commented 2 years ago

Extending to Date.now is not needed anymore now that the range is set explicitly to match hours_to_show

Mmm, I don't agree, and I'll show you and example as soon as I can, but we can ignore it for now.

dbuezas commented 2 years ago

FTR, extend to now is back. Although not strictly required for showing the right xaxis range anymore, It didn't feel right without (as you noted)