termdash icon indicating copy to clipboard operation
termdash copied to clipboard
verified publisher icon mum4k

Support Y-axis custom labels

Open slok opened this issue 6 years ago • 3 comments

Hi!

At this moment, X-axis supports a slice of labels to customize what is rendered on the X-axis.

I haven't seen this for the Y-axis. If this is not implemented, would be nice to have the same feature for the Y-axis. The motivation behind this is to have the ability to do something similar to this:

A simple (without having too much knowledge about the internals of the library) possible solution could be to allow setting a "transforming" function that accepts the y axis value/number as an argument and returns the string to render on the Axis. This way we could convert the Y value to any unit and display the units also.

What are your thoughts on this? would be this possible?

Thank you!

slok avatar Apr 21 '19 08:04 slok

Hello again @slok,

this is a very good point, indeed the code that places the Y Axis labels is a bit rough and unfinished. However we might be able to do better than to let the users provide custom labels. I am a bit vary of letting the user provide the labels, since it a) exposes the user to the internals of the LineChart and b) would be confusing.

(a) The LineChart owns the logic of allocating values to the Y axis and we might want to change or improve that logic in the future. If we let the user provide labels we might end up in a situation from which we can't get out without breaking API changes. (b) The X axis is a bit different, since it is an integer axis, so the user can clearly say that value number 7 should have label value "foo". The Y axis is a float axis so it isn't clear how to specify which values should have labels.

With that, here is a counter proposal and I would love to get your opinion about this. We could do this change in three steps:

  1. We could expose an option that lets the user specify the unit for the Y axis, e.g. time.Duration. When the unit is specified, it would act as a formatter based on the value and display the appropriate suffix like "s", "ms" or "ns". Over time we could implement more unit-specific formatters. Specifically for time as the unit, we could utilize the existing formatting code in the time library: https://golang.org/src/time/time.go#L669

  2. We could develop an algorithm that given a range of floats and a number of horizontal cells (the height of the Y axis) chooses "pretty numbers" and their locations on the axis. So when the graph would have Y axis range from 0.337 to 1000.95, it would not show these values. It would instead choose labels "0", "500" and "1000" and indicate the appropriate cell for each of the labels.

  3. We could change how the LineChart allocates labels for the Y axis. Currently it uses a greedy approach, allocating one label for the bottom-most cell and then choosing every 4-th cell regardless of the value allocated to this cell. We could swap-replace this to use the algorithm developed in step (2). This would also resolve #95.

I believe that we might have very similar result to the picture you uploaded if we do these three things and we still remain in control of the internals without exposing ourselves to breaking API changes. Let me know what you think.

Secondly, if you agree with this approach, please let me know if you would be interested in helping with any of these steps, in which case I can give exact code pointers.

As always, open to other ideas of course.

mum4k avatar Apr 21 '19 21:04 mum4k

Many thanks for your thoughts and the detailed explanation @mum4k it's a pleasure opening issues in this repository :)

I see your concerns with breaking the API, I have the same opinion.

As always I'm happy to help in whatever I can and I like what you propose, so... I'm in!

Apart from this, I understand that if you only let the users select the unit you are abstracting all the unit formatting to them, an that's very good, but could you explain me (only to understand more deeply) In what way would affect the user letting them set an optional formatter as dependency injection? (using a function or an implementation of some kind of interface).

Best.

slok avatar Apr 22 '19 17:04 slok

Thanks a lot @slok for persisting and rephrasing your question. Turns out I misunderstood your note when you filled this issue.

Indeed there is nothing wrong with letting the user provide an optional formatter, i.e. a function that just takes the float64 value and formats it into a string. I believe your idea is much better than mine in point (1).

I would be happy to accept a PR adding a LineChart option that takes such formatter. We could use this instead of (1) above with the understanding that the LineChart still won't choose "pretty" values until we also do (2) and (3). Optionally we could also develop some common formatter(s), say starting with the time formatter and include them with the LineChart, that is up to you really.

I am really glad you're in and willing to help, your PR was written very cleanly and was easy to review so already looking forward to the next one. If you want, you can send a work-in-progress PR with just the API first (the new option and the formatter func definition) - assuming you would like to get a quick feedback on it.

Now some code pointers you might find useful:

  • The LineChart uses the internal axes library to abstract both of the axes and their labels.
  • The greedy algorithm that places Y labels is here: https://github.com/mum4k/termdash/blob/a9515f27215c8c7f3c0df731c4e86103cb9cdb9d/widgets/linechart/internal/axes/label.go#L70
  • Each label is represented as an instance of this object: https://github.com/mum4k/termdash/blob/a9515f27215c8c7f3c0df731c4e86103cb9cdb9d/widgets/linechart/internal/axes/label.go#L53-L56
  • The value of each label is represented using this object: https://github.com/mum4k/termdash/blob/a9515f27215c8c7f3c0df731c4e86103cb9cdb9d/widgets/linechart/internal/axes/value.go#L26-L27
  • The Value object already has an internal formatter here: https://github.com/mum4k/termdash/blob/a9515f27215c8c7f3c0df731c4e86103cb9cdb9d/widgets/linechart/internal/axes/value.go#L70-L85

So it should be fairly simple to provide a custom one and plumb it through the call stack. The only catch is that the Value object is used by both the Y axis and the X axis.

Lastly all the widgets use the same pattern for options, we should add a new option somewhere here: https://github.com/mum4k/termdash/blob/master/widgets/linechart/options.go

Hope this gives you the pointers you need. Once we finish (1), I will be happy to give more pointers for (2) and (3), but at that point you will likely know all you need about the LineChart implementation.

Feel free to contact me at any time if you will have any additional questions or ideas.

mum4k avatar Apr 23 '19 00:04 mum4k