ENH: expose max_labels in ColorMap #90
Conversation
|
CI failures are not related to this PR. |
There was a problem hiding this comment.
I'm okay with exposing this new argument: it's pretty straight forward, it doesn't yet clutter the __init__ and setting the number of labels is a pretty important part of a colorbar.
I made #93, so you can adapt/extend these test cases based on your changes.
branca/utilities.py
Outdated
| legend_ticks = [] | ||
| for i in legend_values[::spacer]: | ||
| legend_ticks += [i] | ||
| legend_ticks += ['']*(spacer-1) | ||
| legend_ticks += [legend_values[-1]] |
There was a problem hiding this comment.
I understand the desire to always show the last value. But this means we may have differing distances between labels. For example consider values [0, 1, 2, 3] and max_labels=3. Should the ticks then be [0, 2] (current situation, equal distance between values but missing last value) or [0, 2, 3] (your PR, unequal distance between values but including last value)? I don't know which is better at the moment, but in doubt I tend to stick with the status quo. What do you think?
Is there a way to have both? Include the first and last value, and have equal spacing? Or, at least, have the last and second to last label not be too close together.
There was a problem hiding this comment.
That is a good point, I didn't realise that.
Is there a way to have both? Include the first and last value, and have equal spacing?
I don't think so. We're picking from a list of variable length. We're not interpolating values here (and I don't think we should).
I am not sure either which of the options is better. The addition of the last value makes it easier to read a bit but it can cause weird overlaps.
Let's keep the status quo for now. I will revert the addition of the last value.
branca/utilities.py
Outdated
| @@ -44,11 +44,12 @@ def legend_scaler(legend_values, max_labels=10.0): | |||
| if len(legend_values) < max_labels: | |||
| legend_ticks = legend_values | |||
| else: | |||
| spacer = int(math.ceil(len(legend_values)/max_labels)) | |||
| spacer = int(math.ceil(len(legend_values) / (max_labels - 1))) | |||
| legend_ticks = [] | |||
| for i in legend_values[::spacer]: | |||
| legend_ticks += [i] | |||
| legend_ticks += ['']*(spacer-1) | |||
There was a problem hiding this comment.
Not related to your change directly, but I don't understand why we add empty strings in between. There doesn't seem to be a need for it. If the idea is to make the length of legend_ticks correspond to legend_values, it's not even correct since we add one more tick than value.
There was a problem hiding this comment.
Don't know but commenting it out doesn't change anything apparent.
|
I've added basic tests for |
There was a problem hiding this comment.
Changes look good 👍 You may have noticed that we are working on getting the tests running again. When they do, let's make sure this PR passes, then we can merge it. If #95 is not enough and it'll take longer to get the tests working, we can also merge this earlier.
Closes #88
This PR introduces 2 changes to the ColorMap labels.
max_labelskeyword to control how many labels should be shown. At the moment there is a hard-coded value of 10, which often causes overlaps.We are currently working on folium-based plotting to be included in GeoPandas and this is one of the things which would be great to fix to make a smoother user experience.
One question - how do you want to test it? I see that branca tests are essentially written as a check that it the code does not fail.