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

ignore math.NaN() values when drawing plots #67

Merged

Conversation

markusressel
Copy link
Contributor

@markusressel markusressel commented Oct 10, 2024

Drawing graphs with "gaps" in the data currently results in an ugly "0 line":

image

This PR simply ignores values that match math.IsNaN(val), leaving an actual gap in the drawn line:

image

Interestingly this behavior only affects the "braille" type line plot. Not sure whats different about the "dot" mode that makes it work anyway.

I tried to run tests, but it seems like running tests in this repo requires an extensive local setup involving multiple external packages, so I wasn't able to get it running. Let me know if I need to add anything else.

PS: Ignore the bad braille rendering, its caused by the IntelliJ integrated shell. Looks much better in an actual shell.

@markusressel markusressel force-pushed the feature/do-not-draw-math-nan-values branch from b240957 to 8897f2f Compare October 10, 2024 21:35
@navidys
Copy link
Owner

navidys commented Oct 10, 2024

Hi @markusressel

Thanks for the PR looks perfect.

can u please just sign your commits " got commit -s"

Regards

@markusressel markusressel force-pushed the feature/do-not-draw-math-nan-values branch from 8897f2f to 2993b79 Compare October 10, 2024 22:29
@markusressel
Copy link
Contributor Author

markusressel commented Oct 10, 2024

I tried signing via intellij, did it work? No idea how I can tell 😅
EDIT: hmm I guess it didn't, I will try the cli version for now.
EDIT2: I cannot sign the commit because I have not setup a GPG key yet. I have kinda read the pages here:
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits
here:
https://www.jetbrains.com/help/go/2024.2/set-up-GPG-commit-signing.html?Set_up_GPG_commit_signing&keymap=XWin+Proper+Redo#enable-commit-signing
and here:
https://docs.github.com/en/authentication/managing-commit-signature-verification/generating-a-new-gpg-key

but I honestly still have no Idea what to choose. How important is this signing stuff?

@markusressel markusressel force-pushed the feature/do-not-draw-math-nan-values branch from 2993b79 to 3dd7ecf Compare October 10, 2024 22:32
@navidys
Copy link
Owner

navidys commented Oct 10, 2024

Hi @markusressel

It's signed now , thanks

@navidys
Copy link
Owner

navidys commented Oct 10, 2024

Hi @markusressel
Another note, can u run demos/demo/main.go with your changes and attach the screenshot here

Thanks

@markusressel
Copy link
Contributor Author

Before:

image

After (with holes added by myself):

image

Thx for the hint, I found an issue that resulted in flickering between an empty and filled graph, caused by the code that determines the required range.

Note that sparkling graphs are still broken. They still flicker to an empty graph sometimes, and do not leave real gaps. Do you want me to fix them as well?

@markusressel
Copy link
Contributor Author

I think I have fixed sparkline as well. Please take a look at the code changes, I had to change the maxVal == 0 early return logic slightly.

New Image:

image

@markusressel
Copy link
Contributor Author

markusressel commented Oct 10, 2024

One thing I am noticing: The Braille lines seem to be much steeper in some cases than they "should be" if the holes were not present. Ia m not sure if this is simply an artifact of how braille lines are drawn, or an issue with the code. It does not look like the lines shift that much out of place to me, so it could just be related to the braille charaters.

@markusressel markusressel force-pushed the feature/do-not-draw-math-nan-values branch from 3d91181 to 170954e Compare October 10, 2024 23:19
@markusressel
Copy link
Contributor Author

I had accidentally pushed my changes to the demo, its fixed now.

@navidys
Copy link
Owner

navidys commented Oct 11, 2024

Hi @markusressel

The braille graph doesn't look ok in previous screenshots, can u send me the screenshot again with your changes again.

Thanks

@markusressel
Copy link
Contributor Author

markusressel commented Oct 11, 2024

@navidys I am not sure I am following, which ones do you mean?
The screenshots where the graphs look "broken" are with the NaN gaps in the data. Without the NaN gaps they look like they did before this PR.

This is what it looks like without holes:
image

This is including holes:
image

Both images include the changes in this PR.

@navidys
Copy link
Owner

navidys commented Oct 11, 2024

Thanks @markusressel

Is it possible to give me access to your tvxwidgets forked repo also ? Just want to check the sample data your are using

@markusressel
Copy link
Contributor Author

I have not pushed the sample data, since there is only this one branch so far, which is also used for this PR and I didn't want to pollute or modify your demo apps. I can add a separate branch to show how I created the sample data if you want.

I have to go to sleep now though (almost 4 AM here), thx for the quick conversation ❤️

@navidys
Copy link
Owner

navidys commented Oct 11, 2024

Hi @markusressel

Thanks for that , I all appreciate if you can do that and send me the branch name

@markusressel
Copy link
Contributor Author

@navidys I think I know what is causing the "steeper than expected" lines:

The calcBrailleLines function calculates braille points based on the "previous" and "current" value within the graph data. The simple logic I have added smply skips the calculation of lheight (the "current" one), but also leaves the previousHeight value as is. This means that if there is a gap in the data, the height of the last non-NaN data point is connected to the first non-NaN data point after the gap. I may have a solution to this problem, I will give it a try.

Until then, you can checkout this branch to play around with both of the PRs, including data that has gaps:

https://github.com/markusressel/tvxwidgets/tree/feature/math-nan-examples

@markusressel
Copy link
Contributor Author

markusressel commented Oct 11, 2024

@navidys can a braille only be drawn using at least two data points? I know the plot.setBrailleLine() needs two, but would it be possible to draw a single point as well? Its not exceptionally important, but not drawing single points would leave out valid data points.

EDIT: Yes it is possible, using plot.setBraillePoint(). I have pushed a commit to fix single valid data points as well. Looks like this:

image

@markusressel
Copy link
Contributor Author

markusressel commented Oct 11, 2024

This is a screenshot of the last commit without gaps (only the sin/cos graph):
image

This is the same commit but with random gaps:
image

And with other, smaller gaps:
image

for j, val := range line[1:] {
lheight := int((val / plot.maxVal) * float64(height-1))
if math.IsNaN(val) {
Copy link
Contributor Author

@markusressel markusressel Oct 11, 2024

Choose a reason for hiding this comment

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

This code could probably be written in a simpler way if it wasn't for the special handling of the first data point.
Since it works as is I didn't want to change it though.
Feel free to optimize.

@navidys
Copy link
Owner

navidys commented Oct 11, 2024

Hi @markusressel

The golangci-lint is failing.

Can u please fix those failure, if not you can give me access to your branch and I can fix them easily.

To run golangci-lint locally on your node

1- make .install.golanci-lint
2- make lint

@markusressel
Copy link
Contributor Author

You do have access to the branch. Its the default:
image

Signed-off-by: Navid Yaghoobi <[email protected]>
@navidys navidys merged commit 89b22f5 into navidys:main Oct 12, 2024
5 checks passed
@markusressel markusressel deleted the feature/do-not-draw-math-nan-values branch October 12, 2024 03:20
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