Feat/time offsets #109

dbuezas · 2022-11-04T10:08:01Z

No description provided.

dbuezas · 2022-11-04T10:09:51Z

@zanna-37, now that you have separated all unrelated fixes, I rebased my offsets branch.

If you have time to close the duplicated datapoints issue, please PR to this branch :). Otherwise, I hope to it sooner or later

dbuezas · 2022-11-04T13:50:35Z

I hope i can get on it sooner or later

dbuezas · 2022-11-06T22:46:52Z

@zanna-37 I rebased now, which includes the better management of the "fake datapoints" and (inspired by you) reworked the git history into "do all the code" and "add a line to the readme" 😁
With that, I think it is almost done 🎉

I think the only thing missing for the offsets is to rework the removeOutsideRange function in the cache.ts file, so it accounts for the offsets. It should work fine now, but it handles the cache very inefficiently (it will delete stuff just to refetch it again an instant later).

Another option is to be less strict removing stuff from the cache. Originally this functionality was there only to ensure that plotly autorange would put in the screen the right xaxis range to respect hours_to_show. Now that's not necessary anymore.

src/plotly-graph-card.ts

zanna-37 · 2022-11-06T23:42:43Z

src/plotly-graph-card.ts

  if (xs.length === 0) return;
  const last = JSON.parse(JSON.stringify(ys[ys.length - 1]));
-  xs.push(new Date());
+  xs.push(new Date(Date.now() + offset));


extendLastDatapointToPresent(...) should be disabled when an offset is set. Those traces reflects values that are not related to the present.

Let's discuss this.
An extreme case is a monthly statistic, without extending, it will look like the plot just stops at end of last month.
IMO, of the behavior is to extend to the present, then it should be consistent or it will surprise the user.
I'll put some example screenshots later

Well, it depends. 😂
When using offsets, we are talking about things that occurred in the past and is now being shifted.
If we talk about forecast it doesn't make sense to extend, because the forecast is referred to a specific point in time, extending it is nonsensical.
However, when talking a tank level, if the level stays at X liters, and it doesn't change, it would make sense to extend it.

Neither is perfect, but maybe the second scenario is more common so we should stick with your vision. Opinions?

True. Give it a try when you test it. I think when you see how a part of the plot disappears as soon as the offset is added you may get the same "this is wrong" feeling.

It is true that it can be seen as a (poor) forecast, but it can also be interpreted as "the last known state at that point".

Let me know what you find out from experimenting. We can always add an option to deactivate the extension

For temperatures and statistics (which are currently timed at the start of their period) extending is surprising, because it extends horizontally between one and two hours but the real temperature didn't stay constant. It looks wierd. If you extended by adding the current state instead of the last fetched one it would look better, but that's IMHO too smart, non generalizable and out of scope.

The "extendLastDatapointToPresent" is not a problem if the offset is moving the trace later.

If one offsets right (positive) by a month and the visible range is a week, the only the section [now - 1month -1 week, now - 1month] will he fetched] . Extending that to now will just add a huge 1 month straight line (visible when you zoom).
If one offsets left (negative), then it is less problematic

if having data past now is not a problem

X axis scaling is now done explicitly, so this is less of an issue now

Note that my proposition makes having an offset (say a small enough offset that you cannot see, but non-zero nonetheless) totally transparent and not changing suddenly the way the trace looks.

I think i got you now. Whatever we do, it should be equivalent to expanding the true last state first and the offseting. (Not offsetting and then expanding the last fetched one, which may not be the latest state of the sensor)

I think i got you now. Whatever we do, it should be equivalent to expanding the true last state first and the offseting.

If there is an existing state right of the visible range, it should probably be fetched for continuity purposes. Extending the last visible state to the right only works when plotting the values piecewise constant. With other interpolation methods the result is wrong.

If there is no such later state, you may want to assume the state did not change by extending to the right, but never past now+offset. I believe this should default to true for normal history and false for statistics, but should be configurable by the user.

Sadly, I don't think HA has a suitable API to fetch « whatever is the next point after this time » so the first paragraph may be infeasible at least for sensors updating very irregularly.

never past now+offset

absolutly

I believe this should default to true for normal history

Agree

and false for statistics

It took me a second to realise why, but I agree now too

but should be configurable by the user.

Also agree. Luckily very easy to implement with the new better structured code.

I don't think HA has a suitable API to fetch « whatever is the next point after this time »

Yeah, I didn't find such an API either.

I'll give this a try now and finally merge this PR

zanna-37

Offset should be considered on src/plotly-graph-card.ts, rows 171-176.

https://github.com/dbuezas/lovelace-plotly-graph-card/pull/109/files#diff-c0352c6cdab9f9d6e1251e51657ccfc84873e2c7b361622d9aa4094c04c11093R171-R176

dbuezas · 2022-11-07T06:25:13Z

Offset should be considered on src/plotly-graph-card.ts, rows 171-176.

https://github.com/dbuezas/lovelace-plotly-graph-card/pull/109/files#diff-c0352c6cdab9f9d6e1251e51657ccfc84873e2c7b361622d9aa4094c04c11093R171-R176

The cache doesn't store an offset history. The offset is applied in the getter in cache.ts/getHistory(Entity)

Meaning, it shouldn't be necessary to consider when adding it from the present in refres_interval: auto

dbuezas · 2022-11-12T20:00:58Z

Config for extending works.

type: custom:plotly-graph-dev
entities:
  - entity: sensor.electricity_total
    extend_to_present: false
    line:
      width: 10
  - entity: sensor.electricity_total
    extend_to_present: true 
hours_to_show: 0.03
update_interval: 1

I think this PR can be merged.
@zanna-37 Should I merge or wait? If you still intend to review the readme of offsets let me know! :)

zanna-37 · 2022-11-12T20:10:28Z

Yes, I'll write the README for offset, maybe tomorrow morning.

Regarding the last code changes, ff you want to wait I can give a look at the code.

dbuezas · 2022-11-13T05:56:30Z

Yes, I'll write the README for offset, maybe tomorrow morning.

Thanks :) !!!

Regarding the last code changes, ff you want to wait I can give a look at the code.

Will be very appreciated! I removed the cache clearing code (*), but had to still add it before plotting to ensure the yaxis auto range applies to the visible section. (So if you reset, the data won't be deleted from cache, but will still not be plotted)

(*) It's kind of hard when combined with offsets, as the same entity may be shown with different offsets in the same plot.

zanna-37 · 2022-11-13T09:59:21Z

It's kind of hard when combined with offsets, as the same entity may be shown with different offsets in the same plot.

It's difficult for me to review the code since I'm losing track of what methods works on the actual values of the offsetted ones.

By the way, it might be a good idea to use entityname+offset as the cache key.

dbuezas · 2022-11-13T10:07:45Z

it might be a good idea to use entityname+offset

The idea is to not fetch the same values twice. The entity key determines the raw data (more or less), and the offsets are applied only before plotting. Multiple traces can be derived from the same cached entry (each with different offsets, lambdas, etc). That's why the cache key only holds the entity name, attribute and statistic type and period.

dbuezas · 2022-11-13T10:09:04Z

what methods works on the actual values of (or?) the offsetted ones.

The offset is only applied to decide what to fetch, and then right before plotting. I think i may remove the offset logic from the cache to make that more evident

dbuezas · 2022-11-13T10:10:12Z

In any case, I'm fairly confident about the offset logic, so the readme would be the most valuable contribution :)

dbuezas · 2022-11-13T10:16:06Z

But you can point out at something specific that is confusing, I'll gladly try to improve it :)

zanna-37 · 2022-11-13T11:23:12Z

Either we keep the cache as points that will be plotted (offset already applied) and in this case we keep separate traces for the same entity with different offset.
OR
We keep the raw entity in the cache but we must be careful about merging and deleting the out-of-range.

so the readme would be the most valuable contribution

I know, it's coming.

point out at something specific

it's just documentation, for methods and vars, but don't worry, I know it's a burden and the problem is mine, not yours 😂

dbuezas · 2022-11-13T11:41:22Z

OR
We keep the raw entity in the cache but we must be careful about merging and deleting the out-of-range.

100%. That's the approach i took in the PR. To solve the deleting problem, i just stopped deleting the cache, but filter the "post processed" data before giving it to plotlyjs.

I just saw that there is a bug (plotlyjs again), where the autorrange button (or double clicking) autorranges to the widest past scales instead of the visible data. It is probably storing that as a side effect somewhere, instead of traversing the data again. I'll try to work around that next.

zanna-37 · 2022-11-13T13:24:26Z

I just stopped deleting the cache

So when the graph stays there for too long we are filling the memory indefinitely. Right?

dbuezas · 2022-11-13T15:02:34Z

I just stopped deleting the cache

So when the graph stays there for too long we are filling the memory indefinitely. Right?

Yes, that's the caveat. And slower operations (filtering happens linearly, no binary search for the boundaries or anything of the sort.
I don't think it should be too tragic though. Worse case we could clear the cache every hour.
Maybe i refactor the cache eventually and implement it better. At the moment, the same info will ve fetched more than once if you plot multiple attributes of the same entity

dbuezas · 2022-11-13T16:11:48Z

readme.md

+  - entity: sensor.actual_temperature
+```
+
+### Extend_to_present


@FrnchFrgg
Here's the toggle for extend_to_present with the defaults you suggested

Clearing out-of-view data was used to ensure xaxis autoranging would show the expected time range in the screen, but xaxis autoranging is not used anymore, and this technique is incompatible with offsets.

This makes auto ranging after resetting behave as it used to (only show as much as zoomed out since the last reset)

zanna-37 · 2022-11-13T09:40:29Z

readme.md

+type: custom:plotly-graph
+entities:
+  - entity: sensor.weather_24h_forecast
+    mode: "markers"


Side note: is mode documented somewhere?

Not within this repo's readme, it's a plotlyjs feature, so it's in their docs

zanna-37 · 2022-11-14T06:11:19Z

README ready at #142.

src/plotly-graph-card.ts

dbuezas · 2023-01-28T15:40:48Z

@zanna-37 fyi: in v3, the offsets are called time_offset. I had to rename the property to avoid a collision with a plotlyjs config to offset bar charts.

dbuezas force-pushed the feat/time-offsets branch 2 times, most recently from 30c1b72 to 7f695c2 Compare November 6, 2022 22:39

zanna-37 suggested changes Nov 6, 2022

View reviewed changes

zanna-37 suggested changes Nov 7, 2022

View reviewed changes

dbuezas force-pushed the master branch from eca7b73 to 206fcca Compare November 7, 2022 19:49

dbuezas force-pushed the feat/time-offsets branch 2 times, most recently from dd97508 to 160c7f2 Compare November 12, 2022 19:49

dbuezas marked this pull request as draft November 13, 2022 16:06

dbuezas commented Nov 13, 2022

View reviewed changes

dbuezas force-pushed the feat/time-offsets branch from 9afe6d3 to 7c99607 Compare November 13, 2022 16:12

dbuezas added 4 commits November 13, 2022 20:54

Added offsets. Work In Progress

7dbe2b2

Added offset yaml to readme

60c2e4a

Don't clear out-of-view data from the cache

d8ac252

Clearing out-of-view data was used to ensure xaxis autoranging would show the expected time range in the screen, but xaxis autoranging is not used anymore, and this technique is incompatible with offsets.

Add extend_to_present option to entities

89528b1

dbuezas added 2 commits November 13, 2022 20:54

Reintroduced range out-of-screen data cleaning, but out of the cache

923076f

Clear cache when exiting browsing mode (e.g scroll, zoom)

7fd272b

This makes auto ranging after resetting behave as it used to (only show as much as zoomed out since the last reset)

dbuezas force-pushed the feat/time-offsets branch from 7c99607 to 7fd272b Compare November 13, 2022 19:54

zanna-37 reviewed Nov 14, 2022

View reviewed changes

Zanna_37 added 2 commits November 14, 2022 07:48

refactor: move images in a separate folder

0417972

docs: add offset to the README

f021a6a

zanna-37 reviewed Nov 14, 2022

View reviewed changes

src/plotly-graph-card.ts Outdated Show resolved Hide resolved

Fix inverted entity.extend_to_presentdefault

8a40c4f

dbuezas force-pushed the feat/time-offsets branch from bd8fe5d to 8a40c4f Compare November 14, 2022 17:22

dbuezas marked this pull request as ready for review November 15, 2022 19:47

dbuezas merged commit be039f3 into master Nov 15, 2022

dbuezas changed the title ~~Feat/time offsets (WIP)~~ Feat/time offsets Nov 15, 2022

dbuezas mentioned this pull request Nov 15, 2022

Feat/time offsets #145

Merged

dbuezas deleted the feat/time-offsets branch November 15, 2022 21:10

Uh oh!

Feat/time offsets #109

Feat/time offsets #109

Uh oh!

Conversation

dbuezas commented Nov 4, 2022

Uh oh!

dbuezas commented Nov 4, 2022

Uh oh!

dbuezas commented Nov 4, 2022

Uh oh!

dbuezas commented Nov 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbuezas Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanna-37 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbuezas commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbuezas commented Nov 12, 2022

Uh oh!

zanna-37 commented Nov 12, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

zanna-37 commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

zanna-37 commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

zanna-37 commented Nov 13, 2022

Uh oh!

dbuezas commented Nov 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanna-37 commented Nov 14, 2022

Uh oh!

Uh oh!

dbuezas commented Jan 28, 2023

Uh oh!

Uh oh!

dbuezas Nov 12, 2022 •

edited

Loading

zanna-37 left a comment •

edited

Loading

dbuezas commented Nov 7, 2022 •

edited

Loading