fix(area_charts): correctly represent baseline with negative data points by markov00 · Pull Request #896 · elastic/elastic-charts

markov00 · 2020-11-06T18:29:41Z

Summary

This PR fixes the rendering issue when using negative values in area charts. The correct approach
is to draw the area with a zero baseline (or dummy one baseline for log y scale). This correctly
represent the sense of area for a single data point.

fix #893

Note for reviewers

It's easier to check the changes if you review them by commit:

the first commit is an initial code cleanup to consider the correct baseline for negative values
the second one moves the renderArea, renderPoints, renderLine etc methods into their respective files to reduce the size of the utils.ts file and increase readability
the third commit fix and refactor the areas of uncertainty for lines/area/points when using log scales

Another thing to notice: the change related to the log scale also changed the way we render areas in the [-1,1] range. I've adopted a different strategy that removes areas/points from showing in that range (possibly adjustable via theme/configuration in a future PR). This adjustment will require the implementation of the #63 request.

Checklist

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

This commit fix the rendering issue when using negative values in area charts. The correct approach is to draw the area with a zero baseline (or dummy one baseline for log y scale). This correctly represent the sense of area for a single data point. fix elastic#893

codecov-io · 2020-11-06T18:35:20Z

Codecov Report

Merging #896 (eca4136) into master (d288208) will decrease coverage by 0.00%.
The diff coverage is 89.08%.

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
- Coverage   69.65%   69.65%   -0.01%     
==========================================
  Files         335      340       +5     
  Lines       10905    10950      +45     
  Branches     2269     2276       +7     
==========================================
+ Hits         7596     7627      +31     
- Misses       3295     3309      +14     
  Partials       14       14

Flag	Coverage Δ
unittests	`69.65% <89.08%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
src/scales/scale_continuous.ts	`95.97% <77.77%> (-1.92%)`	⬇️
src/chart_types/xy_chart/rendering/bars.ts	`82.47% <82.47%> (ø)`
src/chart_types/xy_chart/rendering/points.ts	`89.18% <89.18%> (ø)`
src/chart_types/xy_chart/rendering/area.ts	`91.17% <91.17%> (ø)`
src/chart_types/xy_chart/rendering/utils.ts	`94.38% <94.38%> (ø)`
src/chart_types/xy_chart/rendering/line.ts	`94.73% <94.73%> (ø)`
src/chart_types/xy_chart/renderer/canvas/areas.ts	`30.76% <100.00%> (ø)`
src/chart_types/xy_chart/renderer/canvas/bars.ts	`100.00% <100.00%> (ø)`
...rc/chart_types/xy_chart/renderer/canvas/bubbles.ts	`68.75% <100.00%> (ø)`
src/chart_types/xy_chart/renderer/canvas/lines.ts	`40.00% <100.00%> (ø)`
... and 11 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 d288208...eca4136. Read the comment docs.

nickofthyme

Code changes LGTM. Tested locally and everything functions as expected.

Why does the band disappear when using log scale? 👇

Also, I noticed that for positive and negative values the log scale just shows the positive values. Is there a way to logically/correctly show positive and negative values when using log scale? Like the symmetric log scale, @monfera was talking about?

nickofthyme · 2020-11-10T15:43:08Z

src/chart_types/xy_chart/rendering/rendering.ts

+        if (isLogScale) {
+          return yScale.scaleOrThrow(1);
+        }
+        return yScale.scaleOrThrow(0);


Would these just return a consistent pixel value like 0 rather than scaling the values?

Also, the 1 should be LOG_MIN_ABS_DOMAIN right?

already solved on eca4136

src/chart_types/xy_chart/rendering/bars.ts

src/chart_types/xy_chart/rendering/utils.ts

src/chart_types/xy_chart/rendering/line.ts

src/chart_types/xy_chart/rendering/utils.ts

src/mocks/series/utils.ts

src/scales/scale_continuous.ts

rshen91

Sorry about the delay in reviewing! These changes look great to me - I appreciate the additional stories and clarity in reducing file sizes.

markov00 · 2020-11-11T09:04:01Z

Why does the band disappear when using log scale? 👇

This happens because the lowest point is below the threshold of 1 (currently 0.9). In this case, the band is not defined and is removed from the path

# [24.1.0](v24.0.0...v24.1.0) (2020-11-24) ### Bug Fixes * **area_charts:** correctly represent baseline with negative data points ([#896](#896)) ([d1243f1](d1243f1)) * **legend:** legend sizes with ordinal data ([#867](#867)) ([7559e0d](7559e0d)), closes [#811](#811) * render orphan data points on lines and areas ([#900](#900)) ([0be282b](0be282b)), closes [#783](#783) * specs swaps correctly reflected in state ([#901](#901)) ([7fba882](7fba882)) ### Features * **legend:** allow legend text to be copyable ([#877](#877)) ([9cd3459](9cd3459)), closes [#710](#710) * allow clearing series colors from memory ([#899](#899)) ([ab1af38](ab1af38)) * merge series domain with the domain of another group ([#912](#912)) ([325b013](325b013)) * small multiples for XY charts (alpha) ([#793](#793)) ([d288208](d288208)), closes [#500](#500) [#500](#500)

markov00 · 2020-11-24T14:52:18Z

🎉 This PR is included in version 24.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Domain bounds should be used in the sense of zooming in/out with a viewport on the chart. A regression of this behavior slipped through due to a [PR](#896) causing data to be filtered out when applying a custom domain, removing data points from areas and lines. The right behavior is now restored. fix #1129

…#1181) Domain bounds should be used in the sense of zooming in/out with a viewport on the chart. A regression of this behavior slipped through due to a [PR](elastic/elastic-charts#896) causing data to be filtered out when applying a custom domain, removing data points from areas and lines. The right behavior is now restored. fix opensearch-project#1129

refactor: split rendering function into multiple files

57fdb08

markov00 force-pushed the 2020_11_06-fix_negative_areas branch from 41f2a5e to 8cef28f Compare November 10, 2020 13:48

fix: add test and fix areas of uncertainty for log scales

eca4136

markov00 force-pushed the 2020_11_06-fix_negative_areas branch from aa63501 to eca4136 Compare November 10, 2020 14:45

markov00 marked this pull request as ready for review November 10, 2020 15:28

markov00 requested review from nickofthyme and rshen91 November 10, 2020 15:28

nickofthyme approved these changes Nov 10, 2020

View reviewed changes

rshen91 approved these changes Nov 10, 2020

View reviewed changes

markov00 merged commit d1243f1 into elastic:master Nov 11, 2020

markov00 deleted the 2020_11_06-fix_negative_areas branch November 11, 2020 09:18

markov00 added the released Issue released publicly label Nov 24, 2020

th0ger mentioned this pull request Nov 25, 2020

Auto min value on log scale #916

Closed

3 tasks

markov00 mentioned this pull request May 31, 2021

fix(domain): custom domain should not filter data #1181

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(area_charts): correctly represent baseline with negative data points#896

fix(area_charts): correctly represent baseline with negative data points#896
markov00 merged 3 commits intoelastic:masterfrom
markov00:2020_11_06-fix_negative_areas

markov00 commented Nov 6, 2020 •

edited

Loading

Uh oh!

codecov-io commented Nov 6, 2020 •

edited

Loading

Uh oh!

nickofthyme left a comment

Uh oh!

nickofthyme Nov 10, 2020

Uh oh!

nickofthyme Nov 10, 2020

Uh oh!

markov00 Nov 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rshen91 left a comment

Uh oh!

markov00 commented Nov 11, 2020

Uh oh!

markov00 commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

markov00 commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Note for reviewers

Checklist

Uh oh!

codecov-io commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

nickofthyme Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

nickofthyme Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

markov00 Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rshen91 left a comment

Choose a reason for hiding this comment

Uh oh!

markov00 commented Nov 11, 2020

Uh oh!

markov00 commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

markov00 commented Nov 6, 2020 •

edited

Loading

codecov-io commented Nov 6, 2020 •

edited

Loading