fix: clippedRanges when complete dataset is null by anishagg17 · Pull Request #1037 · elastic/elastic-charts

anishagg17 · 2021-02-28T17:49:18Z

Summary

Fixes: #1047

Before

After

Checklist

Delete any items that are not applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios

kibanamachine · 2021-02-28T17:49:19Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

stories/legend/6_hide_legend.tsx

anishagg17 · 2021-02-28T17:52:05Z

I have partially resolved this issue, looking for maintainers' insights on how they would like to proceed with it(there are multiple levels at which data could be filtered).

markov00 · 2021-03-02T10:59:24Z

Hi @anishagg17, thank you for your contribution, we really appreciated it.
We usually discuss the issue to better define the constraints and use cases before start working on it.
Some issues/requests look fine at a first glance but they probably need a better and thoughtful discussion before being processed and implemented, some others may be outdated or invalid.
In this case, the issue was a bit vague: it actually adds an additional behavior to a specific edge case, increasing the number of internally managed edge cases that are not immediately understandable and known from the outside.
I think we should not proceed further on this issue and close it as it is. @nickofthyme what do you think?

nickofthyme · 2021-03-03T16:05:24Z

Hey @anishagg17 Thanks for opening this PR. I can't recall what the impetus was for opening this issue.

I looked through your solution and after thinking more about this issue I am not sure this is something we need to change. The issue is suggesting we show the empty state UI when all values are null. After discussing this with the team, we think this is not the best approach as null values could be useful to visualize the data.

That being said. I do see an issue with how the line is being rendered see #1047. Would you like to take this issue? I can guide you where to look to fix this.

anishagg17 · 2021-03-03T16:17:23Z

Would you like to take this issue?

Will really like to work on it, as it could help me gain more knowledge regarding the codeBase.

I think that I should make changes for #1047 within this pr only. But could you please let me know where to make changes at, I observed several functions, within which the fix could be implemented

elastic-charts/src/chart_types/xy_chart/state/utils/utils.ts

Line 308 in 85b6ed3

function renderGeometries(
elastic-charts/src/chart_types/xy_chart/rendering/line.ts

Line 40 in 85b6ed3

export function renderLine(

nickofthyme · 2021-03-03T17:38:23Z

Sweet! So the idea behind the fitting functions is that there is data that may be null or undefined and we provide a few options on how these values can be filled. Filled values appear with subdued areas and dashed lines. You can see this better in this story.

Screen.Recording.2021-03-03.at.11.15.02.AM.mp4

Since there are possibly curved lines we must render the full line/area path for both the "real" data and the fitted data. Then with these paths, we determine the clippedRanges or the segments along the x-axis that represent fitted data, then this can be used to clip the paths and merge them into a single chart. So an illustration of that would be something like...

Where the red represents the clippedRanges as a list of intervals (i.e. [[0,1], [3,4]]) which is used to clip the "real" path and the blue represents the inverse of the clippedRanges to clip the "fitted" path.

These clippedRanges are computed here for area series...

elastic-charts/src/chart_types/xy_chart/rendering/area.ts

Line 74 in 85b6ed3

const clippedRanges = getClippedRanges(dataSeries.data, xScale, xScaleOffset);

And here for line series...

elastic-charts/src/chart_types/xy_chart/rendering/line.ts

Line 83 in 85b6ed3

const clippedRanges = getClippedRanges(dataSeries.data, xScale, xScaleOffset);

So with the example in the issue, the clipped ranges return [] which clips nothing and hence the issue. So I would look into the getClippedRanges function here...

elastic-charts/src/chart_types/xy_chart/rendering/utils.ts

Lines 67 to 98 in 85b6ed3

    
           export function getClippedRanges(dataset: DataSeriesDatum[], xScale: Scale, xScaleOffset: number): ClippedRanges { 
        
             let firstNonNullX: number | null = null; 
        
             let hasNull = false; 
        
             return dataset.reduce<ClippedRanges>((acc, data) => { 
        
               const xScaled = xScale.scale(data.x); 
        
               if (xScaled === null) { 
        
                 return acc; 
        
               } 
        
               const xValue = xScaled - xScaleOffset + xScale.bandwidth / 2; 
        
               if (isDatumFilled(data)) { 
        
                 const endXValue = xScale.range[1] - xScale.bandwidth * (2 / 3); 
        
                 if (firstNonNullX !== null && xValue === endXValue) { 
        
                   acc.push([firstNonNullX, xValue]); 
        
                 } 
        
                 hasNull = true; 
        
               } else { 
        
                 if (hasNull) { 
        
                   if (firstNonNullX !== null) { 
        
                     acc.push([firstNonNullX, xValue]); 
        
                   } else { 
        
                     acc.push([0, xValue]); 
        
                   } 
        
                   hasNull = false; 
        
                 } 
        
                 firstNonNullX = xValue; 
        
               } 
        
               return acc; 
        
             }, []); 
        
           }

Maybe we could check if all the values are null then return the full domain within the getClippedRanges function or it might need to be done outside of that function (i.e. where that function is called) to have access to the necessary values. I'm not sure.

Let me know if you have any questions about that and if you run into any issues.

nickofthyme · 2021-03-03T17:42:35Z

Sorry forgot to mention, it's totally fine to make the changes in this pr, just remove the filtering logic on the data for #883.

nickofthyme · 2021-03-04T17:17:03Z

jenkins test this please

nickofthyme

Nicely done! Just a few simplifications.

src/chart_types/xy_chart/rendering/utils.ts

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

github-actions · 2021-03-04T18:16:30Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821200 run.

github-actions · 2021-03-04T18:16:31Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1202109 run.

github-actions · 2021-03-04T18:16:32Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821199 run.

github-actions · 2021-03-04T18:16:39Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1202109 run.

github-actions · 2021-03-04T18:16:42Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821200 run.

github-actions · 2021-03-04T18:16:43Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821200 run.

github-actions · 2021-03-04T18:16:43Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821199 run.

github-actions · 2021-03-04T18:16:43Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1821199 run.

github-actions · 2021-03-04T18:16:44Z

The Workflow run is cancelling this PR. It is an earlier duplicate of 1202109 run.

src/chart_types/xy_chart/rendering/utils.ts

nickofthyme · 2021-03-04T18:25:56Z

jenkins test this please

nickofthyme · 2021-03-04T18:29:30Z

jenkins test this please

nickofthyme

LGTM

anishagg17 · 2021-03-04T18:43:50Z

Due to this error, I made a small change.

nickofthyme · 2021-03-04T18:49:28Z

Sorry about all the updating I had to make some updates to master. I'm done now.

nickofthyme · 2021-03-04T18:49:59Z

jenkins test this please

codecov-io · 2021-03-04T19:00:41Z

Codecov Report

Merging #1037 (8ae7792) into master (5bcd1cb) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1037      +/-   ##
==========================================
- Coverage   72.80%   72.73%   -0.07%     
==========================================
  Files         363      379      +16     
  Lines       11242    10854     -388     
  Branches     2445     2218     -227     
==========================================
- Hits         8185     7895     -290     
+ Misses       3043     2855     -188     
- Partials       14      104      +90

Flag	Coverage Δ
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/chart_types/xy_chart/rendering/utils.ts	`93.10% <100.00%> (-1.22%)`	⬇️
src/state/selectors/get_legend_items_labels.ts	`50.00% <0.00%> (-50.00%)`	⬇️
src/utils/dimensions.ts	`71.42% <0.00%> (-28.58%)`	⬇️
src/components/no_results.tsx	`50.00% <0.00%> (-25.00%)`	⬇️
src/chart_types/xy_chart/utils/panel_utils.ts	`72.72% <0.00%> (-19.59%)`	⬇️
src/components/error_boundary/errors.ts	`50.00% <0.00%> (-16.67%)`	⬇️
src/chart_types/xy_chart/rendering/point_style.ts	`87.50% <0.00%> (-12.50%)`	⬇️
...t/state/selectors/compute_small_multiple_scales.ts	`87.50% <0.00%> (-12.50%)`	⬇️
...rt/renderer/dom/annotations/annotation_tooltip.tsx	`79.31% <0.00%> (-10.35%)`	⬇️
src/utils/data_generators/data_generator.ts	`45.45% <0.00%> (-9.10%)`	⬇️
... and 208 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bcd1cb...8ae7792. Read the comment docs.

nickofthyme · 2021-03-04T19:37:39Z

@anishagg17 all good to merge this whenever you want 👍🏼

markov00

LGTM, thanks @anishagg17 for your work on this, we really appreciate it

anishagg17 · 2021-03-04T19:49:56Z

@anishagg17 all good to merge this whenever you want 👍🏼

I can't merge this, Since this is a community submitted pull request.

nickofthyme · 2021-03-04T20:00:18Z

Oh yeah true. Done! Thanks again 🎉

## [25.1.1](v25.1.0...v25.1.1) (2021-03-05) ### Bug Fixes * clippedRanges when complete dataset is null ([#1037](#1037)) ([51418d2](51418d2)) * **tooltip:** allow explicit boundary element ([#1049](#1049)) ([5cf8461](5cf8461))

nickofthyme · 2021-03-05T00:20:13Z

🎉 This PR is included in version 25.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

## [25.1.1](elastic/elastic-charts@v25.1.0...v25.1.1) (2021-03-05) ### Bug Fixes * clippedRanges when complete dataset is null ([opensearch-project#1037](elastic/elastic-charts#1037)) ([e37b4a7](elastic/elastic-charts@e37b4a7)) * **tooltip:** allow explicit boundary element ([opensearch-project#1049](elastic/elastic-charts#1049)) ([118c2b5](elastic/elastic-charts@118c2b5))

Line doesn't render for null values

67c5d21

anishagg17 commented Feb 28, 2021

View reviewed changes

stories/legend/6_hide_legend.tsx Outdated Show resolved Hide resolved

anishagg17 changed the title ~~Line doesn't render for null values~~ feat(xy_charts): line doesn't render for null values Mar 1, 2021

nickofthyme mentioned this pull request Mar 3, 2021

All null values should render no data #883

Closed

anishagg17 added 2 commits March 4, 2021 00:03

fix: clippedRanges when complete dataset is null

2f17814

Merge branch 'master' into xy

4a48ab9

anishagg17 changed the title ~~feat(xy_charts): line doesn't render for null values~~ fix: clippedRanges when complete dataset is null Mar 3, 2021

Update utils.ts

da223cc

nickofthyme requested review from markov00 and nickofthyme March 4, 2021 17:15

nickofthyme added :styling Styling related issue bug Something isn't working labels Mar 4, 2021

nickofthyme reviewed Mar 4, 2021

View reviewed changes

src/chart_types/xy_chart/rendering/utils.ts Outdated Show resolved Hide resolved

src/chart_types/xy_chart/rendering/utils.ts Outdated Show resolved Hide resolved

anishagg17 and others added 2 commits March 4, 2021 23:46

Update src/chart_types/xy_chart/rendering/utils.ts

e0c1062

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

Update src/chart_types/xy_chart/rendering/utils.ts

556bc53

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

nickofthyme reviewed Mar 4, 2021

View reviewed changes

src/chart_types/xy_chart/rendering/utils.ts Outdated Show resolved Hide resolved

Update src/chart_types/xy_chart/rendering/utils.ts

b8e15d6

Merge branch 'master' into xy

775567f

nickofthyme approved these changes Mar 4, 2021

View reviewed changes

TS error fixed

774db7b

anishagg17 and others added 2 commits March 5, 2021 00:15

Merge branch 'master' into xy

9e0e8ee

Merge branch 'master' into xy

8ae7792

markov00 approved these changes Mar 4, 2021

View reviewed changes

nickofthyme merged commit 51418d2 into elastic:master Mar 4, 2021

anishagg17 deleted the xy branch March 4, 2021 20:00

nickofthyme added the released Issue released publicly label Mar 5, 2021

Conversation

anishagg17 commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Feb 28, 2021

Uh oh!

Uh oh!

anishagg17 commented Feb 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markov00 commented Mar 2, 2021

Uh oh!

nickofthyme commented Mar 3, 2021

Uh oh!

anishagg17 commented Mar 3, 2021

Uh oh!

nickofthyme commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickofthyme commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

github-actions bot commented Mar 4, 2021

Uh oh!

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

anishagg17 commented Mar 4, 2021

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

codecov-io commented Mar 4, 2021

Codecov Report

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

anishagg17 commented Mar 4, 2021

Uh oh!

nickofthyme commented Mar 4, 2021

Uh oh!

nickofthyme commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

anishagg17 commented Feb 28, 2021 •

edited

Loading

anishagg17 commented Feb 28, 2021 •

edited

Loading

nickofthyme commented Mar 3, 2021 •

edited

Loading

nickofthyme commented Mar 3, 2021 •

edited

Loading