Improvements to Sankey class #1574

Merged
merged 7 commits into from Dec 7, 2012

Projects

None yet

3 participants

@kdavies4
Contributor
kdavies4 commented Dec 7, 2012

The changes are mostly minor--fixed typos, some docstring reformatting, adjusted figure size in a demo. The most significant change is to the way that path labels are handled. The label strings are now copied properly so that they aren't modified by reference.

@WeatherGod WeatherGod commented on the diff Dec 7, 2012
lib/matplotlib/sankey.py
@@ -741,10 +741,10 @@ def _get_angle(a, r):
kwds = dict(s=patchlabel, ha='center', va='center')
text = self.ax.text(*offset, **kwds)
if False: # Debug
- print "llpath\n", llpath
- print "ulpath\n", self._revert(ulpath)
- print "urpath\n", urpath
- print "lrpath\n", self._revert(lrpath)
+ print("llpath\n", llpath)
@WeatherGod
WeatherGod Dec 7, 2012 Member

I don't see a from __future__ import print_function. We need to target py2.x when we code, and allow for 2to3 to take care of any py3k issues for us.

@kdavies4
kdavies4 Dec 7, 2012 Contributor

Thanks. I included this in commit 92575e2 below.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Dec 7, 2012
lib/matplotlib/sankey.py
@@ -755,22 +755,18 @@ def _get_angle(a, r):
self.ax.add_patch(patch)
# Add the path labels.
- for i, (number, angle) in enumerate(zip(flows, angles)):
- if labels[i] is None or angle is None:
- labels[i] = ''
+ texts = []
+ for i, (number, angle, label, location) in enumerate(zip(flows, angles,
@WeatherGod
WeatherGod Dec 7, 2012 Member

We could get rid of the enumeration here, right? Also, just to make sure, there is a subtle difference in result. Previously, this for-loop would update the labels list, while the new code would not. This may not be of consequence, but it is a difference.

@kdavies4
kdavies4 Dec 7, 2012 Contributor

Good catch--I removed the enumeration (commit de1f1f4). The second for loop can be merged without any problems. I think that the separation was from a previous hack.

@WeatherGod
Member

Besides the minor issues I have noted, this change seems mostly harmless and improves code readability and improves the docs a bit.

@dmcdougall
Member

This is fine. +1.

@WeatherGod WeatherGod merged commit 3e6e735 into matplotlib:master Dec 7, 2012

1 check passed

default The Travis build passed
Details
@dmcdougall dmcdougall referenced this pull request Dec 7, 2012
Closed

Sankey5 #1407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment