Skip to content

Conversation

@zanna-37
Copy link
Contributor

@zanna-37 zanna-37 commented Nov 3, 2022

I separated the fixes present in time-offset and rebased to the latest master.

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

Unfortunately there is still an issue that I'm not able to solve.
The code crashes at this line saying that was is undefined.
I suspect this is related to the card-mod fix (see #99) and the changes made in the constructor (see ade1ba2). Something is not working.

Can you help me?

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

Oh, that's an easy one, which is already fixed in master.
You can add a question mark:
was?.xyz
(Or rebase, but there will be way too many conflicts, which I prefer to solve myseld)

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

Actually, this branch is already up-to-date with master.
And it's not fixed there.

if (is.hours_to_show !== was.hours_to_show) {

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

Actually, this branch is already up-to-date with master. And it's not fixed there.

if (is.hours_to_show !== was.hours_to_show) {

Oh... I remember doing that, I opened too many branches and worked too hurriedly then.
Anyway, that happens the first time the config is saved, it's fair to put a question mark there, I'll check why the types didn't complain later when I can finally sit at the computer for a bit longer

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

By the way, thanks a lot for your work and attention to detail! :)

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

I put the ?. and it doesn't crash, but still it's acting weird.

immagine

This is the last thing I'm not able to solve. If you manage to help me with that, then adding offset and fixing the "removed code" shouldn't be difficult for me. I already have something almost ready.

p.s. This only happens when you move/zoom the graph.

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

#103

That's this one, fix is in master so ignore it

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

But again, I am based on master. 😂
So the issue is still there.

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

But again, I am based on master. 😂 So the issue is still there.

Oh right. 🤦
That's odd... Can you put the yaml from that issue in a HA and check if you get the ghosting when you scroll/zoom? Either the fix broke (lol), it is incomplete, or there is another issue here

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

Will you leave some words & pics in the readme too? :)

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

Will you leave some words & pics in the readme too? :)

Yes but let's leave this PR for fix only, so no need for pics.
Offset-related features will come in another one.

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

Gotcha. Sorry for the low attention, my new born doesn't allow me to focus on anything at the moment.
Let me know when to merge

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

Don't worry at all. =)
This PR should be ready, EXCEPT that the ghost traces are still there.

  • On v1.5.3
    • I don't experience any ghost traces. Not in my graphs nor in the example you provided.
  • On master (now at 2550b71)
    • I don't experience any ghost traces. Not in my graphs nor in the example you provided.
  • On this branch (now at 2b7a4d0)
    • I only experience ghost traces on my graphs (and only where there are gaps). I don't have issues with the example you provided

In my opinion, something in this PR is causing that. And my guess is "dependencies".
If I use the master's (now at 2550b71) package-lock, everything works.

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 3, 2022

Believe me, give me time and I'll find that bad dependency. I'm almost there but I need to go now. I'll see if I can find it tomorrow.

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

Believe me, give me time and I'll find that bad dependency.
The stubborn succeed 💪
It does look very similar to the bug solved by the "fix lonely data points" function, which was also masked by not manually setting the xaxis range.
I spend many hours banging my head to the wall until I did the following:

  • Add a console.log in a lambda to grab the x and y arrays and paste them in vscode
  • Created a lambda that ignores the sensor data, and return those hard coded arrays instead
  • (At this point you have a yaml with which somebody else can also reproduce the bug and help in the bug hunt)
  • Then I isolated the data that causes the plot to break by incrementally removing chunks of the data
  • Finally, it became obvious that plotly loses track of sections of the plot, as soon as it sees a lonely data point sandwiched between two points with "null" in the y axis (which is the way to represent a gap in the trace)

I am inclined to thing you found a similar issue

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

If I use the master's (now at 2550b71) package-lock, everything works.

I updated plotly could that be it?

@dbuezas dbuezas marked this pull request as ready for review November 3, 2022 22:01
@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

I managed to reproduce the bug you found too, but with an attribute. It looks like undefined is the new null.
When a device is unavailable and we try to get the attribute, we of course get an undefined. Plotly treats it exactly as a null (make a gap), and has exactly the same bug.

My "lonely datapoint" fix seems to work fine once extended to cover undefined too.

@dbuezas
Copy link
Owner

dbuezas commented Nov 3, 2022

But the bug you describe seems to be a different thing. Let me know if Fix ghost traces: Extend patchLonelyDatapoints to cover undefined too has any effect on it

@dbuezas dbuezas merged commit b40e641 into dbuezas:master Nov 4, 2022
@dbuezas
Copy link
Owner

dbuezas commented Nov 4, 2022

I will deploy this already, even if it is the case that there is some trace ghosting issues, it is less broken than the latest release

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 4, 2022

I disagree but, your repo, your rules 😂

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 4, 2022

Btw, your last fix doesn't solve my ghost traces.
The only way is to downgrade plotly and then start from there.
I'll do another pull request this evening.

@dbuezas
Copy link
Owner

dbuezas commented Nov 4, 2022

I disagree but, your repo, your rules 😂

Haha, why not? Is it because the bug you see with the newest plotly version?
I didn't intend to go full dictator, I just have a bit of time this morning and wanted to close some lose ends :)

Thanks A LOT for your work!

@dbuezas
Copy link
Owner

dbuezas commented Nov 4, 2022

BTW If you want, you can paste here a file with the dump of the x and y arrays and we can look at it together. There must be some weird value or combination thereof (maybe a NaN or Infinite?)

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 5, 2022

Believe me, give me time and I'll find that bad dependency. I'm almost there but I need to go now. I'll see if I can find it tomorrow.

It is probably "@types/plotly.js": "^2.12.8", or "plotly.js": "^2.16.1". However, after rebasing to the latest master I can't make it work even downgrading them. So, idk. 😂

One thing for sure is that patchLonelyDatapoints(...) is not fixing the issue, it just masks some particular cases.

@dbuezas
Copy link
Owner

dbuezas commented Nov 5, 2022

Can you dump the data that breals the card and post it here. (E.g add a lambda that spits the x and y arrays to the console)

@zanna-37
Copy link
Contributor Author

zanna-37 commented Nov 9, 2022

Can you dump the data that breaks the card and post it here? (E.g add a lambda that spits the x and y arrays to the console)

Well, I think that is not necessary anymore. Thanks for fixing it, and my compliments for catching the bug. 👌🏻

BTW, did you open an issue on plotly regarding lonely points?

@dbuezas
Copy link
Owner

dbuezas commented Nov 9, 2022

compliments for catching the bug

Thanks! :)
It seems like it took 3 steps to fix this:

  1. fix lonely points inside the range due to "unavailable"
  2. fix lonely points on the edges
  3. same but for "unknown"

They are all the same problem ofc, just tricky to figure out

BTW, did you open an issue on plotly regarding lonely points?

Not yet. I'll do that after I release and confirm the fix for #124 (issue persisted because of state = "unknown", which also acts as a gap). Thanks for the reminder

@zanna-37 zanna-37 deleted the fix/various-fixes branch November 19, 2022 12:59
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