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

improve(stepped): draw step by null-value at begin/end of dataset #694

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Leubeling
Copy link

I have some improvements to draw steps!
problem:
If there are null-values at the beginning or end -> the graph hasn't drawn correct on these points.
fix:
Do not manipulate the idx0/idx1 to draw correct
some improvement:
Draw the first and last line always vertical, because it is possible to start or end in the middle of the graph

@leeoniya
Copy link
Owner

hey @Leubeling thanks for the PR :)

  1. can you add an example that demonstrates your fix to [1]. there are already two stepped demos in there.
  2. please maintain the existing code style. ==, if (, etc.

[1] https://leeoniya.github.io/uPlot/demos/path-gap-clip.html

@Leubeling
Copy link
Author

Hey @leeoniya -> thank you for this project
I have used your examples "Gaps in stepped after/before". I was confused on data[18] - the only different on graph.

So i decided to check this funktion and interpreted:
(align=1) draw the step from this value
(align=-1) draw the step until this value

master-branch

master

improved-branche

fix_stepped

@Leubeling
Copy link
Author

I will make an example to showing my use case.

  • lines and stepps on same graph
  • multiple data with different x-values (normalisation)
  • datapoint can be invalide (gaps)

@Leubeling
Copy link
Author

Leubeling commented May 17, 2022

Her is my use case!

// for example a Switch -> each changes are logged
const dataset01_s_blue = [
    { time: 0, value: 1 },
    { time: 4, value: 0 },
    { time: 6, value: 1 }
]
// for example a digital-Level -> log at fixed intervals and detect errors(gaps)
const dataset02_s_red = [
    { time: 0, value: null }, 
    { time: 1, value: null },
    { time: 2, value: 3 },
    { time: 3, value: 4 },
    { time: 4, value: 5 },
    { time: 5, value: null },
    { time: 6, value: 3 },
    { time: 7, value: 2 },
    { time: 8, value: 1 },
    { time: 9, value: 0 }
]
// some analog -> with flexible intervall
const dataset03_l_green = [
    { time: 1, value: 4.90 },
    { time: 8, value: 1.55 }
]
// some analog -> with flexible intervall and an error(gaps)
const dataset04_l_yellow = [
    { time: 0, value: 1.11 },
    { time: 5, value: null },
    { time: 9, value: 4.44 }
]
const dataset05_sin = [
    { time: 0, value: Math.sin(0 * (Math.PI / 180)) * 5 },
    { time: 1, value: Math.sin(10 * (Math.PI / 180)) * 5 },
    { time: 3, value: Math.sin(30 * (Math.PI / 180)) * 5 },
    { time: 4, value: null },
    { time: 5, value: Math.sin(50 * (Math.PI / 180)) * 5 },
    { time: 7, value: Math.sin(70 * (Math.PI / 180)) * 5 },
    { time: 8, value: Math.sin(80 * (Math.PI / 180)) * 5 },
    { time: 9, value: Math.sin(90 * (Math.PI / 180)) * 5 }
]

master-branch

example_master

improved-branche

example_fix_stepped

@leeoniya
Copy link
Owner

leeoniya commented May 17, 2022

definitely looks like an improvement!

i'll do a more thorough review today or tomorrow. is there a reason for implementing custom normalization instead of using the existing uPlot.join()?

uPlot/dist/uPlot.d.ts

Lines 147 to 148 in ad85ee2

/** outerJoins multiple data tables on table[0] values */
static join(tables: uPlot.AlignedData[], nullModes?: uPlot.JoinNullMode[][]): uPlot.AlignedData;

@Leubeling
Copy link
Author

Leubeling commented May 19, 2022

For me there is a reason, yes! But I forgot to show you in this example! I removed this and use join like all other examples -> because it's always faster.

Now the reason:
Currently my function is really called public add(name: string, set: T, color = 'black')!

  1. so that I can join (like uPlot.join)
  2. and I can extend a series
  3. the timestamp i only use once in my storage (less memory)

@leeoniya
Copy link
Owner

leeoniya commented Jun 6, 2022

hey @Leubeling

sorry for sitting on this PR. what i ended up doing initially (in commits pushed today) is to re-use the linear gap detection logic and extract it from the draw loop for all pathBuilders, this simplifies these loops and brings more uniformity to how gaps are rendered across undefined ranges. you can see the effect in the top two charts of before/after, which now look closer to the ones you show in #694 (comment).

before:
image

after:
image

i've also added your example from #694 (comment) to the gaps/align demo page, in both a "before" and "after" variant. let's take a look at the red step-after series:

[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
[_, _, 3, 4, 5, _, 3, 2, 1, 0]

image

the way this renders currently is consistent with how uplot renders gaps in other paths - the gap starts where the data ends, not where the null begins (there is a gap between index 4 and 6). it does seem reasonable to add an option to each pathBuilder to opt into the behavior that you're trying to attain here. something like gapAlign: -1 | 0 | 1 that would be passed through to findGaps() and handled by an extra align option in that util function, where 0 is "gap-around" (the current behavior), 1 is "gap-after" (the one you want) and -1 is "gap-before" (seems theoretical).

gaps.push(...findGaps(dataX, dataY, idx0, idx1, dir, pixelForX));

the next thing is also one that is currently consistent, where any kind of interpolation is only between known datapoints within the non-null, non-undefined data range (there is never interpolation outside data points to the left an the right), the blue series follows this uniform behaviors. again, i do see the case for wanting to opt into the different behavior for stepped, so an additional option for the stepped builder might be good to add to opt into interpolating into undefined points (that can result from join()) beyond the data edges.

image

i know all of this may seem annoyingly pedantic, but i'd like to make sure there is as much consistency as possible. i'll try to see in the next couple weeks how reasonable it is to do what i'm proposing above. (don't feel pressured to do this yourself in this PR)

@leeoniya
Copy link
Owner

leeoniya commented Jun 6, 2022

i added the alignGaps options to the linear, stepped, and spline pathbuilders in 06882a9, so with alignGaps: 1 you can now get the behavior you're looking for.

image

@Leubeling
Copy link
Author

Leubeling commented Jun 8, 2022

The new function has been simplified really well.
Very good decision to look at the gaps individually.

I took a look at the code and played with it a bit. I commented what I noticed.

the way this renders currently is consistent with how uplot renders gaps in other paths - the gap starts where the data ends, not where the null begins (there is a gap between index 4 and 6). it does seem reasonable to add an option to each pathBuilder to opt into the behavior that you're trying to attain here. something like gapAlign: -1 | 0 | 1 that would be passed through to findGaps() and handled by an extra align option in that util function, where 0 is "gap-around" (the current behavior), 1 is "gap-after" (the one you want) and -1 is "gap-before" (seems theoretical).

For me, I prefere the option series.gaps = () => []. I would say:"you can skip your buildIn gaps.push(...findGaps( ... ));" if these option is existing. If not, then your clipGaps() dosn't respect huge Gaps added by the users series.gaps!
please check commit

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