Fixed choropleth

[FloChehab]

Closes #971, closes #996 and closes #905.

Other changes:

• I removed the chunk below, as I think this check adds an unwanted constraint (if too many colors are asked then it will fail in branca.utilities.color_brewer):

        if threshold_scale is not None and len(threshold_scale) > 6:	
            raise ValueError('The length of threshold_scale is {}, but it may '	
                             'not be longer than 6.'.format(len(threshold_scale)))  # noqa

• I have updated the docstring.
• I have made few changes linked to the fact that every time color_brewer was used, one extra color was generated.

One thing I am not sure: I have added the nan_fill_color parameter at a logical place in the list of parameters but we might want to change that.

All current tests should pass. But new tests might be welcomed for the new features.

[Conengmo]

Excellent PR @FloChehab! Thanks for your contribution. I don't have any changes you should make, though I replied on your discussion points and left some other comments. There are a few things we have to decide on before merging.

Looking forward to finishing and merging this, it will close many issues at once :)

[Conengmo]

By the way, have you considered working with opacity instead of coloring for missing/nan/out-of-range values?

[FloChehab]

By the way, have you considered working with opacity instead of coloring for missing/nan/out-of-range values?

I think those values should standout that's why I went for 'black', but if someone wants to play with opacity it should be possible with HEX strings nan_fill_color='#ffffff00' (it works, I just tested). We could mention it in the docstring.

[Conengmo]

if someone wants to play with opacity it should be possible with HEX strings

Nice one, indeed adding that to the docstring would be good.

Reminds me we shouldn't forget updating these changes in the example notebooks.

[FloChehab]

I have updated the code accordingly to our discussions, I'll wait for your feedback.

Obviously, CI will fail because of the DeprecationWarning and the new check to see if all values fall into the domain defined by bins. So next, we will have to settle on how to update the current tests. Then, what new tests to add. And finally, as you said, give new examples.

[FloChehab]

By the way, while we are working on choropleth, we could also allow the fill_color parameter to be a direct mapping of colors for each bin defined. This would enable the creation of more custom maps (eg: with two colors).

[Conengmo]

we could also allow the fill_color parameter to be a direct mapping of colors for each bin defined

That's not a bad idea. Is that something you would want to do yourself? In any case, I suggest to do that in a new PR after this one, to make this one manageable.

[Conengmo]

Neat! Nice work. I think this is a well rounded PR, with fixing the binning and missing values.

So, what's next? Updating the examples I would say. Only the notebook tests failed, probably because we deprecated threshold_scale.

I know of two notebooks that have choropleth examples, but there may be others. These two are also the notebooks that failed the Travis build.

https://nbviewer.jupyter.org/github/python-visualization/folium/blob/master/examples/GeoJSON_and_choropleth.ipynb

https://nbviewer.jupyter.org/github/python-visualization/folium/blob/master/examples/Quickstart.ipynb

Another thing would be to look at the tests and see if we can change/add something for the new and updated functionality.

[FloChehab]

I have updated the notebooks and added one test (I did my best to come up with something straightforward for testing the new functionalities, it might not be up to your standards).

Also, I am wondering why are the notebooks stored on Github with runned cells? (I feel that it is quite a waste of space, and they are not all up-to-date)

[Conengmo]

Link to nbviewer for your notebook changes:
http://nbviewer.jupyter.org/github/FloChehab/folium/blob/fix_choropleth/examples/Quickstart.ipynb
http://nbviewer.jupyter.org/github/FloChehab/folium/blob/fix_choropleth/examples/GeoJSON_and_choropleth.ipynb

I have updated the notebooks and added one test

Good! Notebooks look good and your test seems decent. Thanks. I think we're done with this PR then! What do you think, good to go?

I am wondering why are the notebooks stored on Github with runned cells

Good question. It's so they are immediately viewable with something like nbviewer.jupyter.org. I don't really like the diffs being unreadable. We do let Travis test if they are renderable though.

Do you have an idea how the way we deal with the notebooks could be improved?

[FloChehab]

I would consider the PR as done on my side too.

I hadn't taken a look at nbviewer before and therefore, I get why notebooks are stored with run cells in the repo.

I am more used to Gitlab CI, and in such context I would convert the notebooks to html in the pipeline and store them as job artefacts (which can be directly accessible using a simple URL). I don't know if this could be possible on Github + Travis.

[ocefpaf]

I would consider the PR as done on my side too.

I hadn't taken a look at nbviewer before and therefore, I get why notebooks are stored with run cells in the repo.

I am more used to Gitlab CI, and in such context I would convert the notebooks to html in the pipeline and store them as job artefacts (which can be directly accessible using a simple URL). I don't know if this could be possible on Github + Travis.

It is and I used to do that but nbviewer is quite handy, it reduces the maintenance steps and we don't waste CIs running the notebooks.

[ocefpaf]

BTW, locally I use nbdime to check notebooks diffs on git. It is quite handy to review notebooks commits.

[Conengmo]

Merged! Thanks @FloChehab, I've enjoyed working with you.

[FloChehab]

@Conengmo you are welcome ! It was a pleasure working with you too (you took quite some time discussing, commenting and giving advice !)

It might be my only contribution to Folium: we had to use Folium for a course at university when we faced the now fixed bugs related to choropleth; I came up with some nasty fix and wanted to have a more durable fix for the whole community 😄 Anyway, it was very nice to collaborate !

[ReidWeb]

Has a version with this changeset been published yet? Or is there a way to easily install the pre-release version? Would quite like to utilise the updated version for something.

[ocefpaf]

Has a version with this changeset been published yet? Or is there a way to easily install the pre-release version? Would quite like to utilise the updated version for something.

Not yet. Planning on a new release for next week.