Colorbar tick formatting

[wwymak]

wwymak [9:52 PM]

• implementation for adding a tick formatting option to the colorbars in geoplot and scatterplot as an alternative to the default scientific notation.
• options for this field are:
  • None (the default scientific notation)
  • string in one of the bokeh formats which gets converted to a NumeralTickFormatter
  • an instance of one of the subclasses of bokeh.models.TickFormatter, which gets passed as is to the colorbar options.

@PatrikHlobil this works for the geoplot but I am not sure what your testing procedure is (and also what you would like me to do for the documentation?)

[PatrikHlobil]

@wwymak:

Cool, thanks a lot for your effort. I have a comment on your code: I think we can simplify this even more. Consider importing only:

from bokeh.models import TickFormatter

then you can make the check like:
def get_tick_formatter(formatter_arg):

def get_tick_formatter(formatter_arg):

    if issubclass(type(formatter_arg), TickFormatter) :
        return formatter_arg
    elif isinstance(formatter_arg, str):
        return NumeralTickFormatter(format=formatter_arg)
    else:
       raise ValueError("<colorbar_tick_format> parameter only accepts a string or a objects of bokeh.models.formatters.")

Then this is a really small & nice integration of the feature. About tests, I haven't so far integrated all my tests for geoplots so far in the project.

Could you write a 1 or 2 nice examples for the documentation (README.md), maybe one with the string and one with a formatter? Then, we could use these examples as test code for this feature!

Cheers, Patrik

[wwymak]

ah, yeap, you are right-- isinstance returns True also for the parent class (I thought you need to be more specific than just checking for TickFormatter).

Have also updated the documentation with the 2 examples of customising the colorbar tick labels.

[PatrikHlobil]

Bokeh.models.formatters

[PatrikHlobil]

Nicely done. Thanks for the extensive documentation also. If you correct my minor comments, also don’t forget to add your name to the CONTRIBUTORS.md file 👌🏼.

Cheers Patrik

[PatrikHlobil]

Nice example 😉

[PatrikHlobil]

Shouldn’t this be issubclass here?

[wwymak]

no, because issubclass requires the argument to be a class, so if you pass in e.g. issubclass("0.0a", TickFormatter) it will throw a TypeError issubclass() arg 1 must be a class. However, isinstance(formatter_arg, TickFormatter) works

[PatrikHlobil]

Yes, but you can use:

issubclass(a.__class__, TickFormatter)

Just a matter of taste. I just think that with issubclass it is a bit clearer what happens here. Do whatever you like 😉

[wwymak]

👍 sure. Have changed it to issubclass.

[PatrikHlobil]

Cool, this looks perfect for me. We should also add some tests, but we could do this in another pr if you want.

Did you run the tests already? Otherwise I have to check it out and would then like to merge the branch 😉

[wwymak]

Did you run the tests already?

Hmm, only test_GeoPandasBokeh.py -- do you want me to run the other ones as well?

We should also add some tests, but we could do this in another pr if you want

Yeah, that might be easier 😉

[PatrikHlobil]

@wwymak : thanks again for helping me improve Pandas Bokeh. Do you plan to implement the tests yourself? The tests are very rudimentary at the moment and basically only check if the provided examples of the documentation run (better than nothing 😔).

[wwymak]

@PatrikHlobil I am happy to add tests, but I am not quite sure what's your testing policy? (as far as I can tell for geoplots you are mostly testing it doesn't throw an error when it plots ?) Or do you add visual tests as well?

[PatrikHlobil]

@wwymak: you are right, I basically check if the Generation an html Export of the plots break on changes. Therefore, I use at the moment just the examples of the README.md. For Geoplots, I just implemented 2 tests so far and I haven’t found time to implement the rest. There are no direct visual tests so far, but the tests create html files with the test plots under the Tests/plot folder. However, I do only check them before releasing a new version on PyPI.

When I would start now with the problem, I would write it in a better testable way, but in the moment I am glad if the examples run and one can see obvious breaking changes.

If you want and have the time, you could also implement the remaining Geoplots of the documentation in the tests 😊