Allow empty values on line chart #185

slok · 2019-04-14T08:19:01Z

This PR Fixes #184.

It will allow having values on the line chart that will not be displayed if they are missing. To do so it will use the math.NaN float64 value.

If this is combined with adaptive Y-axis option, the graphs that have empty gaps and high values with small variance will be more accurate regarding the Y axis (because they will not start at 0).

In this example, we can see that we don't have metrics around 10:03-10:05 making the graph render values as 0 and making Y axis start at 0:

Instead of representing the missing metrics as 0, with this change and using math.NaN, the Y axis has a better range around the min and max values making the graph more accurate.

…e displayed as empty cells Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

coveralls · 2019-04-14T08:24:03Z

Pull Request Test Coverage Report for Build 1052

26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 92.535%

Totals
Change from base Build 1032:	0.02%
Covered Lines:	6644
Relevant Lines:	7180

💛 - Coveralls

mum4k

Great work and thank you for also updating the changelog and the tests.

mum4k · 2019-04-15T02:13:25Z

internal/numbers/numbers.go

 	for _, v := range values {
+		if math.IsNaN(v) {


I think we might have a corner case now - if all the values are NaN, this function will return min==math.MaxFloat64, max==-1 * math.MaxFloat64.

Wondering what the right thing to do is, but considering that the min and max cannot be determined from NaN values, should we return min==NaN, max==NaN?

We probably should also document the corner case in the doc comment and add a corresponding test case.

You are right, maybe we could return some default values for min and max in this case, for example (-1,1), or (0,1). What do you think?

Thinking about this more - this might not be the right place to do that. This function is fairly generic "numerical" function and has more users than just the LineChart. I do agree with you that the behavior of LineChart should be to set some default min and max if all values are NaN.

How about we keep this function generic and return min==NaN, max==NaN when all values are NaN to indicate to all possible callers that the min and max cannot be determined? And then the LineChart internally can interpret the min==NaN, max==NaN as min==0, max==0. Note the LineChart should handle both min and max equal to zero.

mum4k · 2019-04-15T02:18:13Z

widgets/linechart/linechart_test.go

@@ -960,6 +960,53 @@ func TestLineChartDraws(t *testing.T) {
 				return ft
 			},
 		},
+		{
+			desc:   "more values than capacity, X rescales with NaN values ignored",


There are a couple more interesting cases we might want to test:

all the values are NaN.

the very first visible value is NaN.

the very last visible value is NaN.

I would appreciate if you would consider adding the test cases. However I know this is a lot of work, so if you want you can just confirm that the corner cases work and I will be happy to add the missing test coverage after we merge your PR in.

If you do choose to add the tests, we could consider adding them cases with less values in the series (just the smallest amount we need for the test case). That should make them easier to write.

You are right, no problem! I'll add them.

mum4k · 2019-04-15T02:19:06Z

CHANGELOG.md

@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - The grid.Builder API now allows users to specify options for intermediate
  containers, i.e. containers that don't have widgets, but represent rows and
  columns.
+- Line chart widget now allows `math.NaN` values to represent "no value" (values


One more request in relation to the CLA - can you please explicitly confirm on this PR that I have your permission to merge this into the master branch in the next release?

@mum4k has permission to merge this PR in master branch in the next release.

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

slok · 2019-04-17T11:44:08Z

Ready for a new check!

I've added 2 things as you noted:

numbers.MinMax now returns NaN min and max when all values are NaN.
LineChart defaults to 0 when the min or max is NaN.

mum4k

Looks great, thank you very much for the contribution.

mum4k · 2019-04-17T21:54:21Z

internal/numbers/numbers.go

@@ -109,21 +109,33 @@ func Round(x float64) float64 {

 // MinMax returns the smallest and the largest value among the provided values.
 // Returns (0, 0) if there are no values.
+// Ignores NaN values. Allowing NaN values could led in a corner case where all


(nit) Possible typo.

"could led" Vs. "could lead"

mum4k · 2019-04-17T21:57:21Z

internal/numbers/numbers_test.go

 	}

 	for _, tc := range tests {
 		t.Run(tc.desc, func(t *testing.T) {
 			gotMin, gotMax := MinMax(tc.values)
-			if gotMin != tc.wantMin || gotMax != tc.wantMax {
+			// Different assertion for NaN cases.
+			if (math.IsNaN(tc.wantMin) && !math.IsNaN(gotMin)) ||


I wonder - can we simplify this by using pretty.Compare?

if diff := pretty.Compare(tc.wantMin, gotMin); diff != "" { t.Errorf("MinMax => unexpected min, diff (-want, +got):\n %s", diff) } if diff := pretty.Compare(tc.wantMax, gotMax); diff != "" { t.Errorf("MinMax => unexpected max, diff (-want, +got):\n %s", diff) }

I didn't try to use pretty on floats before, but hopefully it does the right thing.

Much better. I wasn't sure if you wanted to use something different than standard conditionals to check floats.

I think pretty is fine even for simple types assuming it works, we use it in majority of tests in this project already.

mum4k · 2019-04-17T22:00:08Z

widgets/linechart/linechart.go

@@ -519,3 +531,22 @@ func (lc *LineChart) maxXValue() int {
 	}
 	return maxLen - 1
 }
+
+const (


(optional) Since these consts are only used within the minMax function below, we could as well define them inside the function. Encapsulating them in might give a better explanation of what they are for.

Or we could as well not use the consts and just go with literal zeroes, up to you.

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

slok · 2019-04-18T18:36:30Z

Ready for a new review!

Sorry for all the time is requiring this PR, thank you.

mum4k · 2019-04-18T18:42:17Z

Looks good, thank you for all the work on this PR!

Allow 'no values' on linechart represented by NaN float64 that will b…

42c2731

…e displayed as empty cells Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

googlebot added the cla: yes label Apr 14, 2019

Update changelog

6cf2caf

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

mum4k requested changes Apr 15, 2019

View reviewed changes

slok added 2 commits April 17, 2019 13:40

Make MinMax return NaN when all values are NaN

9b25e35

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

LineChart defaults to 0 mix and max when these are NaN

b54694e

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

mum4k approved these changes Apr 17, 2019

View reviewed changes

slok added 2 commits April 18, 2019 20:32

Simplify MinMax result float checks on test

2ae5ca1

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

Remove not required constants from linechart minMax wrapper

53abc10

Signed-off-by: Xabier Larrakoetxea <slok69@gmail.com>

mum4k merged commit 544632f into mum4k:devel Apr 18, 2019

mum4k mentioned this pull request Apr 29, 2019

Release v0.9.0 #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow empty values on line chart #185

Allow empty values on line chart #185

slok commented Apr 14, 2019

coveralls commented Apr 14, 2019 •

edited

Loading

mum4k left a comment

mum4k Apr 15, 2019

slok Apr 15, 2019 •

edited

Loading

mum4k Apr 15, 2019

mum4k Apr 15, 2019

slok Apr 15, 2019

mum4k Apr 15, 2019

slok Apr 15, 2019

slok commented Apr 17, 2019

mum4k left a comment

mum4k Apr 17, 2019

mum4k Apr 17, 2019

slok Apr 18, 2019

mum4k Apr 18, 2019

mum4k Apr 17, 2019

slok commented Apr 18, 2019

mum4k commented Apr 18, 2019

Allow empty values on line chart #185

Allow empty values on line chart #185

Conversation

slok commented Apr 14, 2019

coveralls commented Apr 14, 2019 • edited Loading

Pull Request Test Coverage Report for Build 1052

💛 - Coveralls

mum4k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slok Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slok commented Apr 17, 2019

mum4k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slok commented Apr 18, 2019

mum4k commented Apr 18, 2019

coveralls commented Apr 14, 2019 •

edited

Loading

slok Apr 15, 2019 •

edited

Loading