Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Y-axis custom labels #187

Open
slok opened this issue Apr 21, 2019 · 3 comments
Open

Support Y-axis custom labels #187

slok opened this issue Apr 21, 2019 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@slok
Copy link
Contributor

slok commented Apr 21, 2019

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!

@mum4k
Copy link
Owner

mum4k commented Apr 21, 2019

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 LineChart should limit the use of decimal digits on the Y axis #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 mum4k added the enhancement New feature or request label Apr 21, 2019
@slok
Copy link
Contributor Author

slok commented Apr 22, 2019

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.

@mum4k
Copy link
Owner

mum4k commented Apr 23, 2019

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:
    func yLabels(scale *YScale, labelWidth int) ([]*Label, error) {
  • Each label is represented as an instance of this object:
    // Label is one value label on an axis.
    type Label struct {
    // Value if the value to be displayed.
    Value *Value
  • The value of each label is represented using this object:
    // Value represents one value.
    type Value struct {
  • The Value object already has an internal formatter here:
    // Text returns textual representation of the value.
    func (v *Value) Text() string {
    if v.text != "" {
    return v.text
    }
    if math.Ceil(v.Rounded) == v.Rounded {
    return fmt.Sprintf("%.0f", v.Rounded)
    }
    format := fmt.Sprintf("%%.%df", v.NonZeroDecimals+v.ZeroDecimals)
    t := fmt.Sprintf(format, v.Rounded)
    if len(t) > 10 {
    t = fmt.Sprintf("%.2e", v.Rounded)
    }
    return t
    }

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants