Repository: geopandas/geopandas

Issue:
REF: replace _mapclasssify_choro with mapclassify.classify()
Next release of mapclassify will have new high-level API allowing to call all classifiers from a single function. It replaces our custom `_mapclasssify_choro`.

See https://github.com/pysal/mapclassify/pull/90

Once it is released, we should remove our custom function and rely on this.

https://github.com/geopandas/geopandas/blob/924cdf65c7c15b01749d1cdd036c5c291e87b0f4/geopandas/plotting.py#L833

Discussion:
jorisvandenbossche: That enables a really nice code clean-up ! ;-) 

Now, since it's only released just yesterday, I might personally prefer to wait a bit longer with it? 
(eg if we target doing 0.9 with the coming month-ish, we could also directly merge it afterwards?)
martinfleis: > if we target doing 0.9 with the coming month-ish, we could also directly merge it afterwards?

Happy to wait for a release and target this for 0.10.

Patch:
diff --git a/ci/envs/37-minimal.yaml b/ci/envs/37-minimal.yaml
index 0b6fd994..9e3e1a01 100644
--- a/ci/envs/37-minimal.yaml
+++ b/ci/envs/37-minimal.yaml
@@ -19,7 +19,7 @@ dependencies:
   - rtree
   - matplotlib
   - matplotlib=3.1
-  - mapclassify>=2.2.0
+  # - mapclassify=2.4.0 - doesn't build due to conflicts
   - geopy
   - SQLalchemy
   - libspatialite
diff --git a/ci/envs/37-pd10.yaml b/ci/envs/37-pd10.yaml
index e72311f6..3d1d5ff6 100644
--- a/ci/envs/37-pd10.yaml
+++ b/ci/envs/37-pd10.yaml
@@ -25,5 +25,5 @@ dependencies:
   - pip:
       - pyproj==2.3.1
       - geopy
-      - mapclassify==2.2.0
-      - pyarrow
\ No newline at end of file
+      - mapclassify==2.4.0
+      - pyarrow
diff --git a/ci/envs/38-dev.yaml b/ci/envs/38-dev.yaml
index 7a8206e2..bed25392 100644
--- a/ci/envs/38-dev.yaml
+++ b/ci/envs/38-dev.yaml
@@ -21,7 +21,7 @@ dependencies:
   - pyarrow
   - pip:
     - geopy
-    - mapclassify>=2.2.0
+    - mapclassify>=2.4.0
     # dev versions of packages
     - git+https://github.com/numpy/numpy.git@main
     - git+https://github.com/pydata/pandas.git@master
diff --git a/doc/source/getting_started/install.rst b/doc/source/getting_started/install.rst
index 23c5a490..2f65ad71 100644
--- a/doc/source/getting_started/install.rst
+++ b/doc/source/getting_started/install.rst
@@ -155,7 +155,7 @@ Further, optional dependencies are:
 For plotting, these additional packages may be used:
 
 - `matplotlib`_ (>= 3.1.0)
-- `mapclassify`_ (>= 2.2.0)
+- `mapclassify`_ (>= 2.4.0)
 
 
 Using the optional PyGEOS dependency
diff --git a/geopandas/array.py b/geopandas/array.py
index 434246fa..3065621f 100644
--- a/geopandas/array.py
+++ b/geopandas/array.py
@@ -1044,6 +1044,38 @@ class GeometryArray(ExtensionArray):
         else:
             return np.array([g is None for g in self.data], dtype="bool")
 
+    def value_counts(
+        self,
+        dropna: bool = True,
+    ):
+        """
+        Compute a histogram of the counts of non-null values.
+
+        Parameters
+        ----------
+        dropna : bool, default True
+            Don't include counts of NaN
+
+        Returns
+        -------
+        pd.Series
+        """
+
+        # note ExtensionArray usage of value_counts only specifies dropna,
+        # so sort, normalize and bins are not arguments
+        values = to_wkb(self)
+        from pandas import Series, Index
+
+        result = Series(values).value_counts(dropna=dropna)
+        # value_counts converts None to nan, need to convert back for from_wkb to work
+        # note result.index already has object dtype, not geometry
+        # Can't use fillna(None) or Index.putmask, as this gets converted back to nan
+        # for object dtypes
+        result.index = Index(
+            from_wkb(np.where(result.index.isna(), None, result.index))
+        )
+        return result
+
     def unique(self):
         """Compute the ExtensionArray of unique values.
 
diff --git a/geopandas/plotting.py b/geopandas/plotting.py
index 1582b2c3..d460e005 100644
--- a/geopandas/plotting.py
+++ b/geopandas/plotting.py
@@ -747,12 +747,27 @@ GON (((-122.84000 49.00000, -120.0000...
         vmax = len(categories) - 1 if vmax is None else vmax
 
     if scheme is not None:
+        mc_err = (
+            "The 'mapclassify' package (>= 2.4.0) is "
+            "required to use the 'scheme' keyword."
+        )
+        try:
+            import mapclassify
+
+        except ImportError:
+            raise ImportError(mc_err)
+
+        if mapclassify.__version__ < LooseVersion("2.4.0"):
+            raise ImportError(mc_err)
+
         if classification_kwds is None:
             classification_kwds = {}
         if "k" not in classification_kwds:
             classification_kwds["k"] = k
 
-        binning = _mapclassify_choro(values[~nan_idx], scheme, **classification_kwds)
+        binning = mapclassify.classify(
+            np.asarray(values[~nan_idx]), scheme, **classification_kwds
+        )
         # set categorical to True for creating the legend
         categorical = True
         if legend_kwds is not None and "labels" in legend_kwds:
@@ -932,100 +947,3 @@ class GeoplotAccessor(PlotAccessor):
 
     def geo(self, *args, **kwargs):
         return self(kind="geo", *args, **kwargs)
-
-
-def _mapclassify_choro(values, scheme, **classification_kwds):
-    """
-    Wrapper for choropleth schemes from mapclassify for use with plot_dataframe
-
-    Parameters
-    ----------
-    values
-        Series to be plotted
-    scheme : str
-        One of mapclassify classification schemes
-        Options are BoxPlot, EqualInterval, FisherJenks,
-        FisherJenksSampled, HeadTailBreaks, JenksCaspall,
-        JenksCaspallForced, JenksCaspallSampled, MaxP,
-        MaximumBreaks, NaturalBreaks, Quantiles, Percentiles, StdMean,
-        UserDefined
-
-    **classification_kwds : dict
-        Keyword arguments for classification scheme
-        For details see mapclassify documentation:
-        https://pysal.org/mapclassify/api.html
-
-    Returns
-    -------
-    binning
-        Binning objects that holds the Series with values replaced with
-        class identifier and the bins.
-    """
-    try:
-        import mapclassify.classifiers as classifiers
-
-    except ImportError:
-        raise ImportError(
-            "The 'mapclassify' >= 2.2.0 package is required to use the 'scheme' keyword"
-        )
-    from mapclassify import __version__ as mc_version
-
-    if mc_version < LooseVersion("2.2.0"):
-        raise ImportError(
-            "The 'mapclassify' >= 2.2.0 package is required to "
-            "use the 'scheme' keyword"
-        )
-    schemes = {}
-    for classifier in classifiers.CLASSIFIERS:
-        schemes[classifier.lower()] = getattr(classifiers, classifier)
-
-    scheme = scheme.lower()
-
-    # mapclassify < 2.1 cleaned up the scheme names (removing underscores)
-    # trying both to keep compatibility with older versions and provide
-    # compatibility with newer versions of mapclassify
-    oldnew = {
-        "Box_Plot": "BoxPlot",
-        "Equal_Interval": "EqualInterval",
-        "Fisher_Jenks": "FisherJenks",
-        "Fisher_Jenks_Sampled": "FisherJenksSampled",
-        "HeadTail_Breaks": "HeadTailBreaks",
-        "Jenks_Caspall": "JenksCaspall",
-        "Jenks_Caspall_Forced": "JenksCaspallForced",
-        "Jenks_Caspall_Sampled": "JenksCaspallSampled",
-        "Max_P_Plassifier": "MaxP",
-        "Maximum_Breaks": "MaximumBreaks",
-        "Natural_Breaks": "NaturalBreaks",
-        "Std_Mean": "StdMean",
-        "User_Defined": "UserDefined",
-    }
-    scheme_names_mapping = {}
-    scheme_names_mapping.update(
-        {old.lower(): new.lower() for old, new in oldnew.items()}
-    )
-    scheme_names_mapping.update(
-        {new.lower(): old.lower() for old, new in oldnew.items()}
-    )
-
-    try:
-        scheme_class = schemes[scheme]
-    except KeyError:
-        scheme = scheme_names_mapping.get(scheme, scheme)
-        try:
-            scheme_class = schemes[scheme]
-        except KeyError:
-            raise ValueError(
-                "Invalid scheme. Scheme must be in the set: %r" % schemes.keys()
-            )
-
-    if classification_kwds["k"] is not None:
-        from inspect import getfullargspec as getspec
-
-        spec = getspec(scheme_class.__init__)
-        if "k" not in spec.args:
-            del classification_kwds["k"]
-    try:
-        binning = scheme_class(np.asarray(values), **classification_kwds)
-    except TypeError:
-        raise TypeError("Invalid keyword argument for %r " % scheme)
-    return binning


Test patch:
diff --git a/geopandas/tests/test_extension_array.py b/geopandas/tests/test_extension_array.py
index 1b33cccf..b7db16a4 100644
--- a/geopandas/tests/test_extension_array.py
+++ b/geopandas/tests/test_extension_array.py
@@ -434,12 +434,12 @@ class TestComparisonOps(extension_tests.BaseComparisonOpsTests):
 
 
 class TestMethods(extension_tests.BaseMethodsTests):
-    @not_yet_implemented
+    @no_sorting
     @pytest.mark.parametrize("dropna", [True, False])
     def test_value_counts(self, all_data, dropna):
         pass
 
-    @not_yet_implemented
+    @no_sorting
     def test_value_counts_with_normalize(self, data):
         pass
 
diff --git a/geopandas/tests/test_pandas_methods.py b/geopandas/tests/test_pandas_methods.py
index 585b77be..cd29605e 100644
--- a/geopandas/tests/test_pandas_methods.py
+++ b/geopandas/tests/test_pandas_methods.py
@@ -5,7 +5,7 @@ from numpy.testing import assert_array_equal
 import pandas as pd
 
 import shapely
-from shapely.geometry import Point, GeometryCollection
+from shapely.geometry import Point, GeometryCollection, LineString
 
 import geopandas
 from geopandas import GeoDataFrame, GeoSeries
@@ -408,13 +408,31 @@ def test_unique():
     assert_array_equal(s.unique(), exp)
 
 
-@pytest.mark.xfail
 def test_value_counts():
     # each object is considered unique
     s = GeoSeries([Point(0, 0), Point(1, 1), Point(0, 0)])
     res = s.value_counts()
     exp = pd.Series([2, 1], index=[Point(0, 0), Point(1, 1)])
     assert_series_equal(res, exp)
+    # Check crs doesn't make a difference - note it is not kept in output index anyway
+    s2 = GeoSeries([Point(0, 0), Point(1, 1), Point(0, 0)], crs="EPSG:4326")
+    res2 = s2.value_counts()
+    assert_series_equal(res2, exp)
+
+    # check mixed geometry
+    s3 = GeoSeries([Point(0, 0), LineString([[1, 1], [2, 2]]), Point(0, 0)])
+    res3 = s3.value_counts()
+    exp3 = pd.Series([2, 1], index=[Point(0, 0), LineString([[1, 1], [2, 2]])])
+    assert_series_equal(res3, exp3)
+
+    # check None is handled
+    s4 = GeoSeries([Point(0, 0), None, Point(0, 0)])
+    res4 = s4.value_counts(dropna=True)
+    exp4_dropna = pd.Series([2], index=[Point(0, 0)])
+    assert_series_equal(res4, exp4_dropna)
+    exp4_keepna = pd.Series([2, 1], index=[Point(0, 0), None])
+    res4_keepna = s4.value_counts(dropna=False)
+    assert_series_equal(res4_keepna, exp4_keepna)
 
 
 @pytest.mark.xfail(strict=False)
diff --git a/geopandas/tests/test_sindex.py b/geopandas/tests/test_sindex.py
index c625a5d1..d96d6ee6 100644
--- a/geopandas/tests/test_sindex.py
+++ b/geopandas/tests/test_sindex.py
@@ -1,5 +1,3 @@
-import sys
-
 from shapely.geometry import (
     Point,
     Polygon,
@@ -18,7 +16,6 @@ import pytest
 import numpy as np
 
 
-@pytest.mark.skipif(sys.platform.startswith("win"), reason="fails on AppVeyor")
 @pytest.mark.skip_no_sindex
 class TestSeriesSindex:
     def test_has_sindex(self):
@@ -107,7 +104,6 @@ class TestSeriesSindex:
         assert sliced.sindex is not original_index
 
 
-@pytest.mark.skipif(sys.platform.startswith("win"), reason="fails on AppVeyor")
 @pytest.mark.skip_no_sindex
 class TestFrameSindex:
     def setup_method(self):