-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP classify to rgba #211
base: main
Are you sure you want to change the base?
WIP classify to rgba #211
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #211 +/- ##
=======================================
- Coverage 89.4% 88.3% -1.2%
=======================================
Files 8 8
Lines 1088 1107 +19
=======================================
+ Hits 973 977 +4
- Misses 115 130 +15
|
I am think whether this should not be a class or a method on |
i was curious about that, but it seemed like classify was doing the heavy lifting in geopandas, even with the legend? probably no real reason we couldn't do both. The difference is a classifier class doesn't require a camp (edit: though, I guess if its a method it just consumes the cmap) |
ah. but Agree this makes more sense as a method, will move around |
nevermind. If this gets pushed inside the class, there's nowhere to include the nan logic
so I think I would just create a classifier and the rgbas if you need both at the moment |
That is a bit unfortunate to do the classification twice. Especially given some of them can be quite costly. Maybe include NaN tracking in the class? Like your |
yeah, agree. I think the nan-handle logic is only like those three lines. Without touching any of the classification code, we could maybe sneak it into the first step of the binning function so nans are ignored from the outset? |
bins = classify(v.dropna().values, scheme=classifier, k=k).yb | ||
|
||
# create a normalizer using the data's range (not strictly 1-k...) | ||
norm = Normalize(min(bins), max(bins)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear why there is a classifier called prior to here.
It seems to me this is trying to do a classless choropleth map [1].
If that is true, then the first classifier can be omitted and line 258 could become
norm = Normalize(v.min(), v.max())
But maybe I'm missing something here in my understanding?
[1] Tobler, W. (1973). Choropleth maps without class intervals. Geographical
Analysis, 5:262-265
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no the whole idea is to use the classifier to get colors. It might not need that call to Normalize, but we're taking the value, discretizing it to the class bins, then using the bins to get colors from a matplotlib colormap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was classified k=8, fisher jenks. So I'm thinking there should only be 8 colors in the figure, but the additional z axis allows for differentiation between units in the same bin but with different values. The color of the hex fill would be whatever class the hex value is placed into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly. Z and color are disconnnected there. Color is based on Jenks but height is continuous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i.e. all this does is take a cmap and translate bins --> colors)
so if you remove the z-dimension, this function gives you the same choropleth geopandas would give you. But then you can also use extrusion to encode the same or a different variable. A bit more at the end of this
This is definitely useful functionality. I'm not sure that mc is the place for this, for a couple of reasons. First, it makes matplotlib a hard dependency. Second, it seems to consume mc, rather than extend mc , for a particular use case. I can see a few options: Keep in it mc, but move it into a utility module that makes matplotlib a soft dependency. Keep it as suggested with the hard dependency Don't include it in mc but have it be part of a downstream package. Other? |
yeah agree. I'm happy to stick it downstream, but when @martinfleis and I discussed briefly on the last pysal call, we thought it might be useful more broadly (e.g. for geopandas to consume, since its coloring logic is currently a bit arcane). would be easy to make matplotlib a module-level import if we want to go that route |
this adds a function to generate an array of colors from an array of values. I've been toying with pydeck lately, which, basically, requires a column of colors on the df. Looking a bit closer, that's how most (all?) of the viz libraries work, so this is a shot at refactoring geopandas's logic for generating colors.
I can add tests, but wanted to open first to see if folks find this valuable and if it should live here