BUG: Pass style_kwds to parts of multipart geometry

[rwijtvliet]

Fix for Multi-part Lines getting same color but dissimilar width #1384

Closes #1384

[rwijtvliet]

Sorry, didn't mean to close

[rwijtvliet]

This PR makes it easier to check and replicate plot keywords, but I have only implemented it for linewidth and refactored for color.

I tried to put a similar check in place also for alpha, i.e., expanding when list-like, but mpl doesn't seem to accept list-like alpha-values. I didn't want to look into it and expand the scope of the bugfix, so I removed it again.

[martinfleis]

Thank you for the effort! I think it is a good start, but I would like to see this being more general (applied to every supported style kwd). Because otherwise we will always find a case when it is not passed correctly (linestyle...).

You have also introduced one regression, I have left comment where.

[martinfleis]

Can we do it more generally? There is a large number of options we can pass to matplotlib, I would prefer once solution covering them all instead of specifying them one by one. You mentioned that alpha does not work as list-like, so there should be some check on the way (maybe even try/except?).

[rwijtvliet]

I agree the goal should be to check/expand them all. There is a few checks I can imagine, but the easiest would be, to apply only the "expansion" to each keyword in kwargs, and then pass them on to mpl. So: if it is list-like, expand it; if not, just pass it on as-is.

The advantages: we don't need to know anything a priori about these keywords (like, what their allowed values are - floats, or strings, etc.), our code is cleaner/more concise, and we don't replicate anything that mpl already does. We can just let any exceptions bubble up.
This would also apply to / work for the alpha case I mentioned.

The more I think about it, the more sense it makes. What do you think?

2 exceptions/caveats where this would backfire:

• The keyword where a check is necessary is, where there is some overlap/logic that needs to be done, such as in the case of color vs facecolor/edgecolor.
• This would only work if the parameter in question does not accept a list-like "single" value, as happens to be the case also for color if it's an rgba tuple.

I'll look into it.

[martinfleis]

That sounds sensible, but might get tricky.

color/facecolor logic is handled separately now anyway, we can keep it separately.

This would only work if the parameter in question does not accept a list-like "single" value

This is the tricky part. There are style kwds for which we would need to use specific checks, because if it supports list-like single value, we need to repeat that, like

• color (and its variants), but that should be already covered
• linestyle accepts tuple
• markevery accepts tuple/list

There's likely more. But I think it is worth exploring.

[rwijtvliet]

I had another idea. As a function like is_linestyle_like does not seem to exist, how about the following.
For each key-value-pair in kwargs with which e.g. plot_linestring_collection is called, an line is tried to be created like so:

from matplotlib.lines import Line2D
if 'linestyle' in kwargs:
    try:
        l = Line2D([0],[0],linestyle=kwargs['linestyle'])
    except ValueError:
        # expand

My only doubt is with performance.

Do you see any objections? If not I'll go and implement it like this.

[martinfleis]

That does not look like an optimal solution. It comes with an overhead and fallback strategy is never a good way to go.

If the issue is linestyle and markevery only, we can find a way how to check them. linestyle accepts tuple of specific shape (offset, (on_off_seq)) which can be checked for easily and markevery is probably not even applicable to our plots.

So I would add check for is_linestyle_like and go forward with two special cases (this and color).

This should do.

def is_linestyle_like(ls):
    if len(ls) == 2 and isinstance(ls, tuple) and isinstance(ls[1], tuple):
        return True
    return False

edit: Note that this checks for list-like version only, so you need is_list_like first.

[rwijtvliet]

Yes, performance was also my concern.

Ok I've implemented it as suggested, with checks on the specific form that these single list-like values can take.

Also, if an attribute is not allowed to be list-like (alpha for example, and marker - a list of alphas, one for each element in the GeoDataFrame, is not accepted the way a list of linewidths is - (see docs)), this attribute is no longer expanded, even if it's list-like - an error will be thrown either way.

I've gone through the docs as well as the source code, and verified the most common attributes, and it's an improvement over how it was, but I can't guarantee I've been 100% exhaustive.

[rwijtvliet]

added some comments explaining reasoning for certain edits and discussing proposed changes.

[rwijtvliet]

I think this was a bit inconsistent. On the one hand, the value for facecolor/edgecolor was only checked and corrected if color was not specified. Yet on the other hand, the keyword was passed on to the collection regardless.
My proposed solution is to let this function concentrate on expanding the values and let the caller and mpl deal with any overlappingly specified properties. No need to reinvent the wheel

[martinfleis]

Thanks a lot! I am really happy about the way this is done right now.

To make sure this works, we should add (parametric) tests for large number of options, to make sure it is correctly expanded. Can you work on that?

[martinfleis]

Can you check and fix failures of tests? It broke single color options.

[rwijtvliet]

Can check and fix failures of tests? It broke single color options.

I looked into it; the tests that check that a TypeError is raised failed. This is because of the changes I made: incorrect color values are now passed on to mpl, where they cause a ValueError to be raised instead. I've changed the tests to accept both error types.

[martinfleis]

@rwijtvliet just checking here. We'd like to merge this for upcoming 0.8 release. There are still tests to be done (#1385 (review)), are you able/wiling to add them to the PR?

[rwijtvliet]

Hey, thanks for writing. I'm currently not near my development environment; I can try to do it in the coming week. The release is due 30.05. I saw; till when are pull requests possible?
The tests can be relatively light: errors from mpl will be allowed to bubble up, but some could be checked explicitly to test the 'expansion' function.

[martinfleis]

There’s no cut-off date for PRs. If you can make it next week that would be great! Thanks.

[rwijtvliet]

I've gone and created many tests on the most commonly used kwargs (linewidth, linestyle, alpha - color was already extensively tested).

There are currently 3 issues I'm unsure about, 2 of them concerning the plotting of non uniform series. I'll add a comment to the relevant positions in the test code.

[rwijtvliet]

comments for discussion / help / request for review

[martinfleis]

Thanks for the extensive test suite. The issue with mixed geometry types is known, we can deal with that later.

I'll reshuffle tests a bit to have them in individual functions and see what is missing or if it is ready to be merged.

[martinfleis]

Thank you! Alongside fixing the issue, it is also a nice cleanup of plotting functions.

[jorisvandenbossche]

Really nice PR! Thanks a lot @rwijtvliet (and @martinfleis for the review)