allow non-adaptive custom scale

[tcurdt]

allow non-adaptive custom scale

[mum4k]

Agreed, in retrospect tying these two options together was ill-advised. We could try to address this by providing a new option YScaleModeAnchored that will set the Y axis mode back to Anchored. We would have to also add a unit test to confirm this actually works. The code is likely to crash since the current implementation might be depending on the fact that the axis adapts, so some minor changes may be needed deeper in the stack.

Open to other suggestions.

@tcurdt is this something you would be interested in contributing?

[tcurdt]

I am sure I could add the option. Just not sure about the changes need deeper in the stack.

[mum4k]

I would start by just adding the option and a test case that verifies how it behaves. If we are lucky, it will just work. If it won't, I can help dissect the code on the PR that is adding the option.

[tcurdt]

It seems there already is an YScaleModeAnchored on the axes - which just isn't exposed.

https://github.com/mum4k/termdash/blob/master/widgets/linechart/internal/axes/scale.go#L46

I know it would be a breaking change, but frankly speaking just

func YAxisCustomScale(min, max float64) Option {
	return option(func(opts *options) {
		opts.yAxisCustomScale = &customScale{
			min: min,
			max: max,
		}
		//opts.yAxisMode = axes.YScaleModeAdaptive
	})
}

would make so much more sense.

And it would just require to add YAxisAdaptive as another option to get back to the old behaviour.

[tcurdt]

Hm. Odd. Even this refuses to work. Still expands beyond the min/max.

func YAxisCustomScale(min, max float64) Option {
	return option(func(opts *options) {
		opts.yAxisCustomScale = &customScale{
			min: min,
			max: max,
		}
		//opts.yAxisMode = axes.YScaleModeAdaptive
		opts.yAxisMode = axes.YScaleModeAnchored
	})
}

[tcurdt]

Seems like this also need to change

https://github.com/mum4k/termdash/blob/master/widgets/linechart/linechart.go#L174

but then the draw panics

panic: container.Draw =>
error: unable to draw widget *linechart.LineChart:
failure for series first[9] on scale
YScale{Min:Value{Round(-5) => -5}, Max:Value{Round(5) => 5}, Step:Value{Round(0.12658227848101267) => 0.13}, GraphHeight:20}, yd.Scale.ValueToPixel(5.620833778521305) =>
position 82 out of bounds 0 <= pos <= 79

[mum4k]

Sorry it would be great if I could be more helpful, but I am afraid I forgot all the details of the code in question.

Setting a custom scale may need more code changes, because this either breaks some assumptions or simply triggers a bug when the scale is set manually.

I think you likely picked this up from the error message above - when the custom scale was set, the code attempted to place a point outside of the scale (out of bounds). We will have to track down the piece of code that determines the placement and debug it. What I would recommend is taking the very case from the error message above and converting it into a unit test.

[tcurdt]

This seems to be already covered by the existing tests:

    --- FAIL: TestLineChartDraws/custom_Y_scale,_negative_and_positive,_values_don't_fit_so_adjusted (0.00s)
        linechart_test.go:1867: Draw => unexpected error: failure for series first[0] on scale YScale{Min:Value{Round(-200) => -200}, Max:Value{Round(200) => 200}, Step:Value{Round(12.903225806451612) => 12.91}, GraphHeight:8}, yd.Scale.ValueToPixel(-400) => position -15 out of bounds 0 <= pos <= 31, wantDrawErr: false
FAIL

Here the test of course still expresses the wrong expectations.

[tcurdt]

That's with

diff --git a/widgets/linechart/linechart.go b/widgets/linechart/linechart.go
index 5880d45..47e670f 100644
--- a/widgets/linechart/linechart.go
+++ b/widgets/linechart/linechart.go
@@ -172,8 +172,9 @@ func (lc *LineChart) yMinMax() (float64, float64) {
 	}
 
 	if lc.opts.yAxisCustomScale != nil {
-		minimums = append(minimums, lc.opts.yAxisCustomScale.min)
-		maximums = append(maximums, lc.opts.yAxisCustomScale.max)
+		// minimums = append(minimums, lc.opts.yAxisCustomScale.min)
+		// maximums = append(maximums, lc.opts.yAxisCustomScale.max)
+		return lc.opts.yAxisCustomScale.min, lc.opts.yAxisCustomScale.max
 	}
 
 	min, _ := minMax(minimums)
diff --git a/widgets/linechart/options.go b/widgets/linechart/options.go
index 150f28b..c98336b 100644
--- a/widgets/linechart/options.go
+++ b/widgets/linechart/options.go
@@ -156,7 +156,8 @@ func YAxisCustomScale(min, max float64) Option {
 			min: min,
 			max: max,
 		}
-		opts.yAxisMode = axes.YScaleModeAdaptive
+		//opts.yAxisMode = axes.YScaleModeAdaptive
+		opts.yAxisMode = axes.YScaleModeAnchored
 	})
 }
 

[tcurdt]

I guess this something I could work around by limiting the input data - not great but a workaround.
If I find a way to disallow the zooming via mouse.

[mum4k]

@tcurdt I would like to help advise here, but I am having a hard time figuring out the current state of things. Would you mind summarizing the current behavior Vs. the desired one?

[tcurdt]

Alright - the summary is as follows:

• the assignment of opts.yAxisMode = axes.YScaleModeAdaptive should be just removed (unfortunately that does not make it just work)
• removing (or temporarily setting opts.yAxisMode = axes.YScaleModeAnchored) already does show up in the tests as error
• a work around is to enforce the min/max range before handing it to the widget
• the work around falls short when the users zooms or moves the graph (which hopefully can be disabled)

Does that make things clearer?

[mum4k]

I was more after the high level. I think what you commented describes how to adjust the existing test.

I am trying to understand what our goal is. Linechart already supports custom scale, but I think that doesn't work for your use case, because it still scales if a value outside of the range is encountered. Is our goal to work on a mode where that automatic scaling is disabled.

I am not sure how mouse zoom falls into the story. We can indeed add an option to disable zooming, but if we implement this, we need to also make sure that while enabled, mouse zooming works with the custom scale.

Do I understand this correctly?

[tcurdt]

Ah! OK, well, I think it would be nice to be able to disable zooming - but that's really for another ticket I guess.
The idea was just to limit the data myself. So if the user is not able to zoom (s)he wouldn't notice the difference.
It was just an idea of a quick work around.

My use case: I assume you know what an oscilloscope is? I want to implement something similar.
For this it's important that neither the position of 0 nor the scale changes automatically.
It needs to have the same visual reference all the time.
If values go beyond min/max they just leave the visible window - not repositioning or scaling it.

Does that make more sense now?

Frankly speaking it sounds so simple:
It's basically just plotting the data on the canvas with a fix scale.
Maybe the work around could be to write an entirely different more dumb widget instead?

That said, just writing my own widget would still leave it broken IMO.
I do think that opts.yAxisMode = axes.YScaleModeAdaptive should not be the default for YAxisCustomScale.
Which then requires fixing anyway.

WDYT is the best path forward?

[mum4k]

Thank you, I think I now have a good idea of what is needed. I do happen to know what an oscilloscope is, used to play with one years ago, so that explanation was a good one.

I think we should do a few things:

1. We should add a widget option that disables mouse / keyboard zoom. This is very straightforward.
2. I don't think we should change how YAxisCustomScale behaves, because its behavior is pretty well documented. We should instead add a new option like YAxisCustomScaleFixed and document that when values outside of the range are encountered, an empty space is drawn on the linechart. Then we need to modify the code to support this functionality.

Would this support the use case you have in mind?

[tcurdt]

I still think YAxisCustomScale is very much mixing concerns with YAxisAdaptive.
And it seems like XAxisUnscaled would also somehow suggest YAxisUnscaled.

The zooming already provides a way show a slice/window of the data, too.
Isn't that functionality already a more powerful superset of the scaling?
So there might also be some overlap?
Just writing down some random thoughts here.

...but in the end it's of course all your decision - and it sounds like the approach you outlined will also work for me!

[mum4k]

Sorry for not being clear, I do agree with you that YAxisCustomScale is mixing concerns. Please do keep the suggestions coming, this is how we make better software together.

My concern is about breaking a behavior that someone out there might depend on regardless how weird the behavior is in retrospect. Or rather I think we should brake it in a more user friendly way. It would be ok to fix bugs, but this would radically change how an established option behaves and we have no way of figuring out if there are users who depend on it.

Therefore my suggestion is to fix it forward in a compatible manner. We can design a set of new options that unlike YAxisCustomScale behave sanely. We can even mark YAxisCustomScale as deprecated in documentation and emit a warning when it is used indicating that users should move and we might eventually delete it completely, telling users how to migrate to the replacement options. All of this would be acceptable, but it feels that changing the behavior of an established option would create a hard to debug scenario for someone already using it.

What do you think?

[tcurdt]

We can even mark YAxisCustomScale as deprecated in documentation and emit a warning when it is used indicating that users should move and we might eventually delete it completely, telling users how to migrate to the replacement options

That certainly would be the path of good engineering. That said, I've seen other breaking changes in the release notes and the version is pre-1.0. The fix would essentially be a trivial "add this option". So I figured...

But if you want to treat your users well, deprecating certainly is the better way to go! Totally agree.

Taking a step back I am really wondering if the scaling isn't really just setting the initial zoom and anchor.
Maybe everything is already there - we just need to find a good way of expressing it via options.

WDYT?

[mum4k]

Sorry I think I need to clarify one more detail. Breaking changes are indeed ok all the way until we get into post 1.0. What I am trying to prevent is a silent behavior breakage. I.e. it is ok (if we have to) to break code in a way that makes the compiler complain, as that makes users look at the changelog. However a silent change of behavior on an already established option is what I think we should avoid as there is no warning. Users will have to notice the change in the behavior of their application.

You raise a good point, I also think that most of the code that we need is there already and agree that we just need to find the right set of options. Maybe some minor tweaks will be needed. I wish I could advise more, but linechart is one of the more complicated widgets and I no longer remember the details.

[tcurdt]

However a silent change of behavior on an already established option is what I think we should avoid as there is no warning

That's fair.

linechart is one of the more complicated widgets and I no longer remember the details

Maybe it's better I start with a simpler version then and see if they somehow converge.