ronantakizawa/codereview-bench huggingface dataset:
Repository: matplotlib/matplotlib
File: galleries/examples/color/colorbar_histogram.py (Python)

Code before:

import matplotlib.colors as mcolors

# surface data
delta = 0.025
x = y = np.arange(-2.0, 2.0, delta)
X, Y = np.meshgrid(x, y)
Z1 = np.exp(-(((X + 1) * 1.3) ** 2) - ((Y + 1) * 1.3) ** 2)
Z2 = 2.5 * np.exp(-((X - 1) ** 2) - (Y - 1) ** 2)
Z = Z1**0.25 - Z2**0.5

# colormap & normalization
bins = 30
cmap = plt.get_cmap("RdYlBu_r")
bin_edges = np.linspace(Z.min(), Z.max(), bins + 1)
norm = mcolors.BoundaryNorm(bin_edges, cmap.N)

# main plot
fig, ax = plt.subplots(layout="constrained")
im = ax.imshow(Z, cmap=cmap, origin="lower", extent=[-3, 3, -3, 3], norm=norm)

# inset histogram
cax = ax.inset_axes([1.18, 0.02, 0.25, 0.95])  # left, bottom, width, height

# plot histogram
counts, bin_edges = np.histogram(Z, bins=bins)
midpoints = (bin_edges[:-1] + bin_edges[1:]) / 2
distance = midpoints[1] - midpoints[0]
cax.barh(midpoints, counts, height=0.8 * distance, color=cmap(norm(midpoints)))

# styling
cax.spines[:].set_visible(False)
cax.set_yticks(bin_edges)
cax.tick_params(axis="both", which="both", length=0)

plt.show()


Review comment (suggestion):
```suggestion
counts, _ = np.histogram(Z, bins=bin_edges)
```
Sorry, I just realized, we can make this now slightly safer/more obvious by using the existing `bin_edges` and not let `np.histogram` create them anew, hoping they are identical to the explicit calculation above.

Code after:

import matplotlib.colors as mcolors

# surface data
delta = 0.025
x = y = np.arange(-2.0, 2.0, delta)
X, Y = np.meshgrid(x, y)
Z1 = np.exp(-(((X + 1) * 1.3) ** 2) - ((Y + 1) * 1.3) ** 2)
Z2 = 2.5 * np.exp(-((X - 1) ** 2) - (Y - 1) ** 2)
Z = Z1**0.25 - Z2**0.5

# colormap & normalization
bins = 30
cmap = plt.get_cmap("RdYlBu_r")
bin_edges = np.linspace(Z.min(), Z.max(), bins + 1)
norm = mcolors.BoundaryNorm(bin_edges, cmap.N)

# main plot
fig, ax = plt.subplots(layout="constrained")
im = ax.imshow(Z, cmap=cmap, origin="lower", extent=[-3, 3, -3, 3], norm=norm)

# inset histogram
cax = ax.inset_axes([1.18, 0.02, 0.25, 0.95])  # left, bottom, width, height

# plot histogram
counts, _ = np.histogram(Z, bins=bin_edges)
midpoints = (bin_edges[:-1] + bin_edges[1:]) / 2
distance = midpoints[1] - midpoints[0]
cax.barh(midpoints, counts, height=0.8 * distance, color=cmap(norm(midpoints)))

# styling
cax.spines[:].set_visible(False)
cax.set_yticks(bin_edges)
cax.tick_params(axis="both", which="both", length=0)

plt.show()
Repository: matplotlib/matplotlib
File: lib/matplotlib/path.py (Python)

Code before:
    @should_simplify.setter
    def should_simplify(self, should_simplify):
        self._should_simplify = should_simplify

    @property
    def readonly(self):
        """
        `True` if the `Path` is read-only.
        """
        return self._readonly

    def copy(self):
        """
        Return a shallow copy of the `Path`, which will share the
        vertices and codes with the source `Path`.
        """
        return copy.copy(self)

    def __deepcopy__(self, memo=None):
        """
        Return a deepcopy of the `Path`.  The `Path` will not be
        readonly, even if the source `Path` is.
        """
        memo = memo or {}
        if id(self) in memo:
            return memo[id(self)]

        cls = type(self)
        p = cls.__new__(cls)
        memo[id(self)] = p

        for k, v in self.__dict__.items():
            setattr(p, k, copy.deepcopy(v, memo))

        p._readonly = False
        return p

    deepcopy = __deepcopy__

    @classmethod
    def make_compound_path_from_polys(cls, XY):
        """
        Make a compound `Path` object to draw a number of polygons with equal
        numbers of sides.

        .. plot:: gallery/misc/histogram_path.py

        Parameters
        ----------
        XY : (numpolys, numsides, 2) array
        """

Review comment (suggestion):
AFAIK, `memo` is only needed for passing it to `deepcopy()`. So just remove any other use of `memo`.

Code after:
    def should_simplify(self, should_simplify):
        self._should_simplify = should_simplify

    @property
    def readonly(self):
        """
        `True` if the `Path` is read-only.
        """
        return self._readonly

    def copy(self):
        """
        Return a shallow copy of the `Path`, which will share the
        vertices and codes with the source `Path`.
        """
        return copy.copy(self)

    def __deepcopy__(self, memo):
        """
        Return a deepcopy of the `Path`.  The `Path` will not be
        readonly, even if the source `Path` is.
        """
        # Deepcopying arrays (vertices, codes) strips the writeable=False flag.
        cls = type(self)
        memo[id(self)] = p = cls.__new__(cls)

        for k, v in self.__dict__.items():
            setattr(p, k, copy.deepcopy(v, memo))

        p._readonly = False
        return p

    def deepcopy(self, memo=None):
        """
        Return a deep copy of the `Path`.  The `Path` will not be readonly,
        even if the source `Path` is.

        Parameters
        ----------
        memo : dict, optional
            A dictionary to use for memoizing, passed to `copy.deepcopy`.

        Returns
        -------
        Path
            A deep copy of the `Path`, but not readonly.
        """
        return copy.deepcopy(self, memo)

    @classmethod
    def make_compound_path_from_polys(cls, XY):
Repository: matplotlib/matplotlib
File: lib/matplotlib/path.py (Python)

Code before:

    @should_simplify.setter
    def should_simplify(self, should_simplify):
        self._should_simplify = should_simplify

    @property
    def readonly(self):
        """
        `True` if the `Path` is read-only.
        """
        return self._readonly

    def copy(self):
        """
        Return a shallow copy of the `Path`, which will share the
        vertices and codes with the source `Path`.
        """
        return copy.copy(self)

    def __deepcopy__(self, memo=None):
        """
        Return a deepcopy of the `Path`.  The `Path` will not be
        readonly, even if the source `Path` is.
        """
        # Deepcopying arrays (vertices, codes) strips the writeable=False flag.
        if memo is None:
            memo = {}
        cls = type(self)
        memo[id(self)] = p = cls.__new__(cls)

        for k, v in self.__dict__.items():
            setattr(p, k, copy.deepcopy(v, memo))

        p._readonly = False
        return p

    deepcopy = __deepcopy__

    @classmethod
    def make_compound_path_from_polys(cls, XY):
        """
        Make a compound `Path` object to draw a number of polygons with equal
        numbers of sides.

        .. plot:: gallery/misc/histogram_path.py

        Parameters
        ----------
        XY : (numpolys, numsides, 2) array
        """
        # for each poly: 1 for the MOVETO, (numsides-1) for the LINETO, 1 for

Review comment (suggestion):
This is not needed, `memo` is always a dict.

The default value for `memo` is also not needed. The `memo` argument is always passed to `__deepcopy__()`.

Code after:
    @should_simplify.setter
    def should_simplify(self, should_simplify):
        self._should_simplify = should_simplify

    @property
    def readonly(self):
        """
        `True` if the `Path` is read-only.
        """
        return self._readonly

    def copy(self):
        """
        Return a shallow copy of the `Path`, which will share the
        vertices and codes with the source `Path`.
        """
        return copy.copy(self)

    def __deepcopy__(self, memo):
        """
        Return a deepcopy of the `Path`.  The `Path` will not be
        readonly, even if the source `Path` is.
        """
        # Deepcopying arrays (vertices, codes) strips the writeable=False flag.
        cls = type(self)
        memo[id(self)] = p = cls.__new__(cls)

        for k, v in self.__dict__.items():
            setattr(p, k, copy.deepcopy(v, memo))

        p._readonly = False
        return p

    def deepcopy(self, memo=None):
        """
        Return a deep copy of the `Path`.  The `Path` will not be readonly,
        even if the source `Path` is.

        Parameters
        ----------
        memo : dict, optional
            A dictionary to use for memoizing, passed to `copy.deepcopy`.

        Returns
        -------
        Path
            A deep copy of the `Path`, but not readonly.
        """
        return copy.deepcopy(self, memo)

    @classmethod
Repository: matplotlib/matplotlib
File: src/_path.h (C/C++)

Code before:
            --c;
        }
        if (*c == '.') {
            --c;
        }
        try {
            buffer.append(str, c + 1);
        } catch (std::bad_alloc& e) {
            PyMem_Free(str);
            throw e;
        }
        PyMem_Free(str);
    }
}


template <class PathIterator>
bool __convert_to_string(PathIterator &path,
                         int precision,
                         std::array<std::string, 5> &codes,
                         bool postfix,
                         std::string& buffer)
{
    const char format_code = 'f';

    std::array<double, 3> x;
    std::array<double, 3> y;
    double last_x = 0.0;
    double last_y = 0.0;

    unsigned code;

    while ((code = path.vertex(&std::get<0>(x), &std::get<0>(y))) != agg::path_cmd_stop) {
        if (code == CLOSEPOLY) {
            buffer += std::get<4>(codes);
        } else if (code < 5) {
            size_t size = NUM_VERTICES[code];

            for (size_t i = 1; i < size; ++i) {
                unsigned subcode = path.vertex(&x.at(i), &y.at(i));
                if (subcode != code) {
                    return false;
                }
            }

            /* For formats that don't support quad curves, convert to
               cubic curves */
            if (code == CURVE3 && codes[code - 1][0] == '\0') {
                quad2cubic(last_x, last_y, x.at(0), y.at(0), x.at(1), y.at(1), x, y);
                code++;
                size = 3;

Review comment (suggestion):
I think you can still do `&x[0]` no? (or `x.at(0)` if you *really* want bounds checking here; this still reads better than std::get I'd say)

Code after:
        } catch (std::bad_alloc& e) {
            PyMem_Free(str);
            throw e;
        }
        PyMem_Free(str);
    }
}


template <class PathIterator>
bool __convert_to_string(PathIterator &path,
                         int precision,
                         const std::array<std::string, 5> &codes,
                         bool postfix,
                         std::string& buffer)
{
    const char format_code = 'f';

    std::array<double, 3> x;
    std::array<double, 3> y;
    double last_x = 0.0;
    double last_y = 0.0;

    unsigned code;

    while ((code = path.vertex(&std::get<0>(x), &std::get<0>(y))) != agg::path_cmd_stop) {
        if (code == CLOSEPOLY) {
            buffer += std::get<4>(codes);
        } else if (code < 5) {
            size_t size = NUM_VERTICES[code];

            for (size_t i = 1; i < size; ++i) {
                unsigned subcode = path.vertex(&x.at(i), &y.at(i));
                if (subcode != code) {
                    return false;
                }
            }

            /* For formats that don't support quad curves, convert to
               cubic curves */
            if (code == CURVE3 && codes[code - 1][0] == '\0') {
                quad2cubic(last_x, last_y, x.at(0), y.at(0), x.at(1), y.at(1), x, y);
                code++;
                size = 3;
            }

            if (!postfix) {
                buffer += codes.at(code - 1);
                buffer += ' ';
            }
Repository: matplotlib/matplotlib
File: src/_path.h (C/C++)

Code before:
{
    double x0;
    double y0;
    double x1;
    double y1;
    double xm;
    double ym;
};

void reset_limits(extent_limits &e)
{
    e.x0 = std::numeric_limits<double>::infinity();
    e.y0 = std::numeric_limits<double>::infinity();
    e.x1 = -std::numeric_limits<double>::infinity();
    e.y1 = -std::numeric_limits<double>::infinity();
    /* xm and ym are the minimum positive values in the data, used
       by log scaling */
    e.xm = std::numeric_limits<double>::infinity();
    e.ym = std::numeric_limits<double>::infinity();
}

inline void update_limits(double x, double y, extent_limits &e)
{
    if (x < e.x0)
        e.x0 = x;
    if (y < e.y0)
        e.y0 = y;
    if (x > e.x1)
        e.x1 = x;
    if (y > e.y1)
        e.y1 = y;
    /* xm and ym are the minimum positive values in the data, used
       by log scaling */
    if (x > 0.0 && x < e.xm)
        e.xm = x;
    if (y > 0.0 && y < e.ym)
        e.ym = y;
}

template <class PathIterator>
void update_path_extents(PathIterator &path, agg::trans_affine &trans, extent_limits &extents)
{
    typedef agg::conv_transform<PathIterator> transformed_path_t;
    typedef PathNanRemover<transformed_path_t> nan_removed_t;
    double x, y;
    unsigned code;

    transformed_path_t tpath(path, trans);
    nan_removed_t nan_removed(tpath, true, path.has_codes());

    nan_removed.rewind(0);

Review comment (suggestion):
Is the new form equivalent to the old one if either start.x or x is nan?  (Ditto for all other similar changes.)
(It's also possible that the answer is "we never have nans here and it doesn't matter", I haven't checked.)

Code after:
}

struct extent_limits
{
    XY start;
    XY end;
    /* minpos is the minimum positive values in the data; used by log scaling. */
    XY minpos;

    extent_limits() : start{0,0}, end{0,0}, minpos{0,0} {
        reset();
    }

    void reset()
    {
        start.x = std::numeric_limits<double>::infinity();
        start.y = std::numeric_limits<double>::infinity();
        end.x = -std::numeric_limits<double>::infinity();
        end.y = -std::numeric_limits<double>::infinity();
        minpos.x = std::numeric_limits<double>::infinity();
        minpos.y = std::numeric_limits<double>::infinity();
    }

    void update(double x, double y)
    {
        start.x = std::min(start.x, x);
        start.y = std::min(start.y, y);
        end.x = std::max(end.x, x);
        end.y = std::max(end.y, y);
        if (x > 0.0) {
            minpos.x = std::min(minpos.x, x);
        }
        if (y > 0.0) {
            minpos.y = std::min(minpos.y, y);
        }
    }
};

template <class PathIterator>
void update_path_extents(PathIterator &path, agg::trans_affine &trans, extent_limits &extents)
{
    typedef agg::conv_transform<PathIterator> transformed_path_t;
    typedef PathNanRemover<transformed_path_t> nan_removed_t;
    double x, y;
    unsigned code;

    transformed_path_t tpath(path, trans);
    nan_removed_t nan_removed(tpath, true, path.has_codes());

    nan_removed.rewind(0);
Repository: matplotlib/matplotlib
File: lib/matplotlib/tests/test_matplotlib.py (Python)

Code before:
import os
import subprocess
import sys
from unittest.mock import patch
import pytest

import matplotlib
from matplotlib.testing import subprocess_run_for_testing


@pytest.mark.parametrize('version_str, version_tuple', [
    ('3.5.0', (3, 5, 0, 'final', 0)),
    ('3.5.0rc2', (3, 5, 0, 'candidate', 2)),
    ('3.5.0.dev820+g6768ef8c4c', (3, 5, 0, 'alpha', 820)),
    ('3.5.0.post820+g6768ef8c4c', (3, 5, 1, 'alpha', 820)),
])
def test_parse_to_version_info(version_str, version_tuple):
    assert matplotlib._parse_to_version_info(version_str) == version_tuple


@pytest.mark.skipif(sys.platform == "win32",
                    reason="chmod() doesn't work as is on Windows")
@pytest.mark.skipif(sys.platform != "win32" and os.geteuid() == 0,
                    reason="chmod() doesn't work as root")
def test_tmpconfigdir_warning(tmp_path):
    """Test that a warning is emitted if a temporary configdir must be used."""
    mode = os.stat(tmp_path).st_mode
    try:
        os.chmod(tmp_path, 0)
        proc = subprocess_run_for_testing(
            [sys.executable, "-c", "import matplotlib"],

Review comment (suggestion):
`pytest` is third-party and should be in a separate block here.

Code after:
import os
import subprocess
import sys
from unittest.mock import patch

import pytest

import matplotlib
from matplotlib.testing import subprocess_run_for_testing


@pytest.mark.parametrize('version_str, version_tuple', [
    ('3.5.0', (3, 5, 0, 'final', 0)),
    ('3.5.0rc2', (3, 5, 0, 'candidate', 2)),
    ('3.5.0.dev820+g6768ef8c4c', (3, 5, 0, 'alpha', 820)),
    ('3.5.0.post820+g6768ef8c4c', (3, 5, 1, 'alpha', 820)),
])
def test_parse_to_version_info(version_str, version_tuple):
    assert matplotlib._parse_to_version_info(version_str) == version_tuple


@pytest.mark.skipif(sys.platform == "win32",
                    reason="chmod() doesn't work as is on Windows")
@pytest.mark.skipif(sys.platform != "win32" and os.geteuid() == 0,
                    reason="chmod() doesn't work as root")
def test_tmpconfigdir_warning(tmp_path):
    """Test that a warning is emitted if a temporary configdir must be used."""
    mode = os.stat(tmp_path).st_mode
    try:
        os.chmod(tmp_path, 0)
        proc = subprocess_run_for_testing(
Repository: matplotlib/matplotlib
File: lib/matplotlib/tests/test_matplotlib.py (Python)

Code before:
    assert (set(parse('- interactive backends:\n')) ==
            set(backend_registry.list_builtin(BackendFilter.INTERACTIVE)))
    assert (set(parse('- non-interactive backends:\n')) ==
            set(backend_registry.list_builtin(BackendFilter.NON_INTERACTIVE)))


def test_importable_with__OO():
    """
    When using -OO or export PYTHONOPTIMIZE=2, docstrings are discarded,
    this simple test may prevent something like issue #17970.
    """
    program = (
        "import matplotlib as mpl; "
        "import matplotlib.pyplot as plt; "
        "import matplotlib.cbook as cbook; "
        "import matplotlib.patches as mpatches"
    )
    subprocess_run_for_testing(
        [sys.executable, "-OO", "-c", program],
        env={**os.environ, "MPLBACKEND": ""}, check=True
        )


@patch('matplotlib.subprocess.check_output')
def test_get_executable_info_timeout(mock_check_output):
    """
    Test that _get_executable_info raises ExecutableNotFoundError if the
    command times out.
    """

    mock_check_output.side_effect = subprocess.TimeoutExpired(cmd=['mock'], timeout=30)

    with pytest.raises(matplotlib.ExecutableNotFoundError) as exc_info:
        print(matplotlib._get_executable_info.__wrapped__('inkscape'))

    assert "Timed out" in str(exc_info.value)


Review comment (suggestion):
You could use `monkeypatch` here, though I have no preference here.

Code after:

    assert (set(parse('- interactive backends:\n')) ==
            set(backend_registry.list_builtin(BackendFilter.INTERACTIVE)))
    assert (set(parse('- non-interactive backends:\n')) ==
            set(backend_registry.list_builtin(BackendFilter.NON_INTERACTIVE)))


def test_importable_with__OO():
    """
    When using -OO or export PYTHONOPTIMIZE=2, docstrings are discarded,
    this simple test may prevent something like issue #17970.
    """
    program = (
        "import matplotlib as mpl; "
        "import matplotlib.pyplot as plt; "
        "import matplotlib.cbook as cbook; "
        "import matplotlib.patches as mpatches"
    )
    subprocess_run_for_testing(
        [sys.executable, "-OO", "-c", program],
        env={**os.environ, "MPLBACKEND": ""}, check=True
        )


@patch('matplotlib.subprocess.check_output')
def test_get_executable_info_timeout(mock_check_output):
    """
    Test that _get_executable_info raises ExecutableNotFoundError if the
    command times out.
    """

    mock_check_output.side_effect = subprocess.TimeoutExpired(cmd=['mock'], timeout=30)

    with pytest.raises(matplotlib.ExecutableNotFoundError, match='Timed out'):
        matplotlib._get_executable_info.__wrapped__('inkscape')
Repository: matplotlib/matplotlib
File: lib/matplotlib/tests/test_matplotlib.py (Python)

Code before:
    """
    program = (
        "import matplotlib as mpl; "
        "import matplotlib.pyplot as plt; "
        "import matplotlib.cbook as cbook; "
        "import matplotlib.patches as mpatches"
    )
    subprocess_run_for_testing(
        [sys.executable, "-OO", "-c", program],
        env={**os.environ, "MPLBACKEND": ""}, check=True
        )


@patch('matplotlib.subprocess.check_output')
def test_get_executable_info_timeout(mock_check_output):
    """
    Test that _get_executable_info raises ExecutableNotFoundError if the
    command times out.
    """

    mock_check_output.side_effect = subprocess.TimeoutExpired(cmd=['mock'], timeout=30)

    with pytest.raises(matplotlib.ExecutableNotFoundError) as exc_info:
        print(matplotlib._get_executable_info.__wrapped__('inkscape'))

    assert "Timed out" in str(exc_info.value)


Review comment (suggestion):
```suggestion
    with pytest.raises(matplotlib.ExecutableNotFoundError, match='Timed out'):
        print(matplotlib._get_executable_info.__wrapped__('inkscape'))
```

Code after:
    this simple test may prevent something like issue #17970.
    """
    program = (
        "import matplotlib as mpl; "
        "import matplotlib.pyplot as plt; "
        "import matplotlib.cbook as cbook; "
        "import matplotlib.patches as mpatches"
    )
    subprocess_run_for_testing(
        [sys.executable, "-OO", "-c", program],
        env={**os.environ, "MPLBACKEND": ""}, check=True
        )


@patch('matplotlib.subprocess.check_output')
def test_get_executable_info_timeout(mock_check_output):
    """
    Test that _get_executable_info raises ExecutableNotFoundError if the
    command times out.
    """

    mock_check_output.side_effect = subprocess.TimeoutExpired(cmd=['mock'], timeout=30)

    with pytest.raises(matplotlib.ExecutableNotFoundError, match='Timed out'):
        matplotlib._get_executable_info.__wrapped__('inkscape')