ronantakizawa/codereview-bench huggingface dataset:
Repository: scikit-learn/scikit-learn
File: sklearn/utils/_array_api.py (Python)

Code before:

    return out


def _cholesky(covariance, xp):
    if _is_numpy_namespace(xp):
        return scipy.linalg.cholesky(covariance, lower=True)
    else:
        return xp.linalg.cholesky(covariance)


def _linalg_solve(cov_chol, eye_matrix, xp):
    if _is_numpy_namespace(xp):
        return scipy.linalg.solve_triangular(cov_chol, eye_matrix, lower=True)
    else:
        return xp.linalg.solve(cov_chol, eye_matrix)


def _half_multinomial_loss(y, pred, sample_weight=None, xp=None):
    """A version of the multinomial loss that is compatible with the array API"""
    xp, _, device_ = get_namespace_and_device(y, pred, sample_weight)
    log_sum_exp = _logsumexp(pred, axis=1, xp=xp)
    y = xp.asarray(y, dtype=xp.int64, device=device_)
    class_margins = xp.arange(y.shape[0], device=device_) * pred.shape[1]
    label_predictions = xp.take(_ravel(pred), y + class_margins)
    return float(_average(log_sum_exp - label_predictions, weights=sample_weight))


Review comment (suggestion):
```suggestion
    return float(_average(log_sum_exp - label_predictions, weights=sample_weight, xp=xp))
```

`_average`-`xp`-police here :sweat_smile: 

Code after:

    return out


def _cholesky(covariance, xp):
    if _is_numpy_namespace(xp):
        return scipy.linalg.cholesky(covariance, lower=True)
    else:
        return xp.linalg.cholesky(covariance)


def _linalg_solve(cov_chol, eye_matrix, xp):
    if _is_numpy_namespace(xp):
        return scipy.linalg.solve_triangular(cov_chol, eye_matrix, lower=True)
    else:
        return xp.linalg.solve(cov_chol, eye_matrix)


def _half_multinomial_loss(y, pred, sample_weight=None, xp=None):
    """A version of the multinomial loss that is compatible with the array API"""
    xp, _, device_ = get_namespace_and_device(y, pred, sample_weight)
    log_sum_exp = _logsumexp(pred, axis=1, xp=xp)
    y = xp.asarray(y, dtype=xp.int64, device=device_)
    class_margins = xp.arange(y.shape[0], device=device_) * pred.shape[1]
    label_predictions = xp.take(_ravel(pred), y + class_margins)
    return float(
        _average(log_sum_exp - label_predictions, weights=sample_weight, xp=xp)
    )
Repository: scikit-learn/scikit-learn
File: sklearn/metrics/_plot/tests/test_common_curve_display.py (Python)

Code before:
    classifier = LogisticRegression().fit(X, y)

    class SubclassOfDisplay(Display):
        pass

    if constructor == "from_predictions":
        curve = SubclassOfDisplay.from_predictions(y, y_pred)
    else:  # constructor == "from_estimator"
        curve = SubclassOfDisplay.from_estimator(classifier, X, y)

    assert isinstance(curve, SubclassOfDisplay)


# TODO(1.10): Remove once deprecated in all Displays
@pytest.mark.parametrize(
    "Display, display_kwargs",
    [
        (
            PrecisionRecallDisplay,
            {"precision": np.array([1, 0.5, 0]), "recall": np.array([0, 0.5, 1])},
        ),
        (RocCurveDisplay, {"fpr": np.array([0, 0.5, 1]), "tpr": np.array([0, 0.5, 1])}),
    ],
)
def test_display_estimator_name_deprecation(pyplot, Display, display_kwargs):
    """Check deprecation of `estimator_name`."""
    with pytest.warns(FutureWarning, match="`estimator_name` is deprecated in"):
        Display(**display_kwargs, estimator_name="test")


Review comment (suggestion):
We need 2 separate comments here imo because the test won't pass for RocCurve after 1.9 so we can't keep the whole test all along and remove it at once when all classes are done
```suggestion
@pytest.mark.parametrize(
    "Display, display_kwargs",
    [
        # TODO(1.10): Remove
        (
            PrecisionRecallDisplay,
            {"precision": np.array([1, 0.5, 0]), "recall": np.array([0, 0.5, 1])},
        ),
        # TODO(1.9): Remove
        (RocCurveDisplay, {"fpr": np.array([0, 0.5, 1]), "tpr": np.array([0, 0.5, 1])}),
    ],
)
```

Code after:
    classifier = LogisticRegression().fit(X, y)

    class SubclassOfDisplay(Display):
        pass

    if constructor == "from_predictions":
        curve = SubclassOfDisplay.from_predictions(y, y_pred)
    else:  # constructor == "from_estimator"
        curve = SubclassOfDisplay.from_estimator(classifier, X, y)

    assert isinstance(curve, SubclassOfDisplay)


# TODO(1.10): Remove once deprecated in all Displays
@pytest.mark.parametrize(
    "Display, display_kwargs",
    [
        # TODO(1.10): Remove
        (
            PrecisionRecallDisplay,
            {"precision": np.array([1, 0.5, 0]), "recall": np.array([0, 0.5, 1])},
        ),
        # TODO(1.9): Remove
        (RocCurveDisplay, {"fpr": np.array([0, 0.5, 1]), "tpr": np.array([0, 0.5, 1])}),
    ],
)
def test_display_estimator_name_deprecation(pyplot, Display, display_kwargs):
    """Check deprecation of `estimator_name`."""
    with pytest.warns(FutureWarning, match="`estimator_name` is deprecated in"):
        Display(**display_kwargs, estimator_name="test")
Repository: scikit-learn/scikit-learn
File: build_tools/update_environments_and_lock_files.py (Python)

Code before:
logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)
handler = logging.StreamHandler()
logger.addHandler(handler)

TRACE = logging.DEBUG - 5


common_dependencies_without_coverage = [
    "python",
    "numpy",
    "blas",
    "scipy",
    "cython",
    "joblib",
    "threadpoolctl",
    "matplotlib",
    "pandas",
    "pyamg",
    "pytest",
    "pytest-xdist",
    "pillow",
    "pip",
    "ninja",
    "meson-python",
    "pytest-playwright",
]

common_dependencies = common_dependencies_without_coverage + [
    "pytest-cov",
    "coverage",
]

docstring_test_dependencies = ["sphinx", "numpydoc"]

default_package_constraints = {
    # TODO: remove once https://github.com/numpy/numpydoc/issues/638 is fixed
    # and released.
    "numpydoc": "<1.9.0",
    # TODO: remove once when we're using the new way to enable coverage in subprocess
    # introduced in 7.0.0, see https://github.com/pytest-dev/pytest-cov?tab=readme-ov-file#upgrading-from-pytest-cov-63
    "pytest-cov": "<=6.3.0",
}


def remove_from(alist, to_remove):
    return [each for each in alist if each not in to_remove]


build_metadata_list = [
    {

Review comment (performance):
Rather than adding it to all the CI jobs, I think we should add it to a single fast to run CI job.

I am worried that the extra install time / setup time will slow our CI too much for little extra value otherwise (especially for the jobs that are already above 25 min on average)

Code after:
logger = logging.getLogger(__name__)
logger.setLevel(logging.INFO)
handler = logging.StreamHandler()
logger.addHandler(handler)

TRACE = logging.DEBUG - 5


common_dependencies_without_coverage = [
    "python",
    "numpy",
    "blas",
    "scipy",
    "cython",
    "joblib",
    "threadpoolctl",
    "matplotlib",
    "pandas",
    "pyamg",
    "pytest",
    "pytest-xdist",
    "pillow",
    "pip",
    "ninja",
    "meson-python",
]

common_dependencies = common_dependencies_without_coverage + [
    "pytest-cov",
    "coverage",
]

docstring_test_dependencies = ["sphinx", "numpydoc"]

default_package_constraints = {
    # TODO: remove once https://github.com/numpy/numpydoc/issues/638 is fixed
    # and released.
    "numpydoc": "<1.9.0",
    # TODO: remove once when we're using the new way to enable coverage in subprocess
    # introduced in 7.0.0, see https://github.com/pytest-dev/pytest-cov?tab=readme-ov-file#upgrading-from-pytest-cov-63
    "pytest-cov": "<=6.3.0",
}


def remove_from(alist, to_remove):
    return [each for each in alist if each not in to_remove]


build_metadata_list = [
    {
        "name": "pylatest_conda_forge_cuda_array-api_linux-64",
Repository: scikit-learn/scikit-learn
File: build_tools/azure/install.sh (Shell)

Code before:
        # FIXME: temporary fix to link against system libraries on linux
        # https://github.com/scikit-learn/scikit-learn/issues/20640
        export LDFLAGS="$LDFLAGS -Wl,--sysroot=/"
    fi

    if [[ "$PIP_BUILD_ISOLATION" == "true" ]]; then
        # Check that pip can automatically build scikit-learn with the build
        # dependencies specified in pyproject.toml using an isolated build
        # environment:
        pip install --verbose .
    else
        if [[ "$UNAMESTR" == "MINGW64"* ]]; then
           # Needed on Windows CI to compile with Visual Studio compiler
           # otherwise Meson detects a MINGW64 platform and use MINGW64
           # toolchain
           ADDITIONAL_PIP_OPTIONS='-Csetup-args=--vsenv'
        fi
        # Use the pre-installed build dependencies and build directly in the
        # current environment.
        pip install --verbose --no-build-isolation --editable . $ADDITIONAL_PIP_OPTIONS
    fi

    ccache -s || echo "ccache not installed, skipping ccache statistics"
}

setup_playwright() {
    if python -c "import playwright" &>/dev/null; then
        python -m playwright install --with-deps
    fi
}

main() {
    pre_python_environment_install
    python_environment_install_and_activate
    scikit_learn_install
    setup_playwright
}

main


Review comment (suggestion):
```suggestion
setup_playwright_if_installed() {
```

Code after:
        # FIXME: temporary fix to link against system libraries on linux
        # https://github.com/scikit-learn/scikit-learn/issues/20640
        export LDFLAGS="$LDFLAGS -Wl,--sysroot=/"
    fi

    if [[ "$PIP_BUILD_ISOLATION" == "true" ]]; then
        # Check that pip can automatically build scikit-learn with the build
        # dependencies specified in pyproject.toml using an isolated build
        # environment:
        pip install --verbose .
    else
        if [[ "$UNAMESTR" == "MINGW64"* ]]; then
           # Needed on Windows CI to compile with Visual Studio compiler
           # otherwise Meson detects a MINGW64 platform and use MINGW64
           # toolchain
           ADDITIONAL_PIP_OPTIONS='-Csetup-args=--vsenv'
        fi
        # Use the pre-installed build dependencies and build directly in the
        # current environment.
        pip install --verbose --no-build-isolation --editable . $ADDITIONAL_PIP_OPTIONS
    fi

    ccache -s || echo "ccache not installed, skipping ccache statistics"
}

setup_playwright_if_installed() {
    if python -c "import playwright" &>/dev/null; then
        python -m playwright install --with-deps
    fi
}

main() {
    pre_python_environment_install
    python_environment_install_and_activate
    scikit_learn_install
    setup_playwright_if_installed
}

main
Repository: scikit-learn/scikit-learn
File: build_tools/azure/install.sh (Shell)

Code before:
    else
        if [[ "$UNAMESTR" == "MINGW64"* ]]; then
           # Needed on Windows CI to compile with Visual Studio compiler
           # otherwise Meson detects a MINGW64 platform and use MINGW64
           # toolchain
           ADDITIONAL_PIP_OPTIONS='-Csetup-args=--vsenv'
        fi
        # Use the pre-installed build dependencies and build directly in the
        # current environment.
        pip install --verbose --no-build-isolation --editable . $ADDITIONAL_PIP_OPTIONS
    fi

    ccache -s || echo "ccache not installed, skipping ccache statistics"
}

setup_playwright() {
    if python -c "import playwright" &>/dev/null; then
        python -m playwright install --with-deps
    fi
}

main() {
    pre_python_environment_install
    python_environment_install_and_activate
    scikit_learn_install
    setup_playwright
}

main


Review comment (suggestion):
```suggestion
    setup_playwright_if_installed
```

Code after:
    else
        if [[ "$UNAMESTR" == "MINGW64"* ]]; then
           # Needed on Windows CI to compile with Visual Studio compiler
           # otherwise Meson detects a MINGW64 platform and use MINGW64
           # toolchain
           ADDITIONAL_PIP_OPTIONS='-Csetup-args=--vsenv'
        fi
        # Use the pre-installed build dependencies and build directly in the
        # current environment.
        pip install --verbose --no-build-isolation --editable . $ADDITIONAL_PIP_OPTIONS
    fi

    ccache -s || echo "ccache not installed, skipping ccache statistics"
}

setup_playwright_if_installed() {
    if python -c "import playwright" &>/dev/null; then
        python -m playwright install --with-deps
    fi
}

main() {
    pre_python_environment_install
    python_environment_install_and_activate
    scikit_learn_install
    setup_playwright_if_installed
}

main
Repository: scikit-learn/scikit-learn
File: sklearn/utils/_repr_html/tests/test_js.py (Python)

Code before:
from http.server import BaseHTTPRequestHandler, HTTPServer
from pathlib import Path

import pytest


@pytest.fixture(scope="session", autouse=True)
def check_playwright():
    """Skip tests if playwright is not installed.

    This fixture is used by the next fixture (which is autouse) to skip all tests
    if playwright is not installed."""
    return pytest.importorskip("playwright")


@pytest.fixture
def local_server(request):
    """Start a simple HTTP server that serves custom HTML per test.

    Usage :

    ```python
    def test_something(page, local_server):
        url, set_html_response = local_server
        set_html_response("<html>...</html>")
        page.goto(local_server)
        ...
    ```
    """
    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
        s.bind(("127.0.0.1", 0))
        PORT = s.getsockname()[1]

    html_content = "<html><body>Default</body></html>"

    def set_html_response(content):
        nonlocal html_content
        html_content = content

    class Handler(BaseHTTPRequestHandler):
        def do_GET(self):
            self.send_response(200)
            self.send_header("Content-type", "text/html")
            self.end_headers()
            self.wfile.write(html_content.encode("utf-8"))

        # suppress logging
        def log_message(self, format, *args):
            return

    httpd = HTTPServer(("127.0.0.1", PORT), Handler)

Review comment (suggestion):
Should this be:
```suggestion
        page.goto(url)
```


Code after:
from http.server import BaseHTTPRequestHandler, HTTPServer
from pathlib import Path

import pytest


@pytest.fixture(scope="session", autouse=True)
def check_playwright():
    """Skip tests if playwright is not installed.

    This fixture is used by the next fixture (which is autouse) to skip all tests
    if playwright is not installed."""
    return pytest.importorskip("playwright")


@pytest.fixture
def local_server(request):
    """Start a simple HTTP server that serves custom HTML per test.

    Usage :

    ```python
    def test_something(page, local_server):
        url, set_html_response = local_server
        set_html_response("<html>...</html>")
        page.goto(url)
        ...
    ```
    """
    with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
        s.bind(("127.0.0.1", 0))
        PORT = s.getsockname()[1]

    html_content = "<html><body>Default</body></html>"

    def set_html_response(content):
        nonlocal html_content
        html_content = content

    class Handler(BaseHTTPRequestHandler):
        def do_GET(self):
            self.send_response(200)
            self.send_header("Content-type", "text/html")
            self.end_headers()
            self.wfile.write(html_content.encode("utf-8"))

        # suppress logging
        def log_message(self, format, *args):
            return

    httpd = HTTPServer(("127.0.0.1", PORT), Handler)
Repository: scikit-learn/scikit-learn
File: build_tools/azure/get_selected_tests.py (Python)

Code before:
import logging
import os

from get_commit_message import get_commit_message

logger = logging.getLogger(__name__)


def get_selected_tests():
    """Parse the commit message to check if pytest should run only specific tests.

    If so, selected tests will be run with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all".

    The commit message must take the form:
        <title> [all random seeds]
        <test_name_1>
        <test_name_2>
        ...
    """
    if "SELECTED_TESTS" in os.environ or os.environ.get("GITHUB_ACTIONS", False):
        raise RuntimeError(
            f"It seems that the `SELECTED_TESTS` environment variable is set or you are"
            f"in a GitHub Actions context "
            f"(GITHUB_ACTIONS={os.environ.get('GITHUB_ACTIONS', False)})."
            f"Instead, please use directly `SELECTED_TESTS`."
        )

    logger.debug("`selected_tests` coming from Azure")

    commit_message = get_commit_message()

    if "[all random seeds]" in commit_message:
        selected_tests = commit_message.split("[all random seeds]")[1].strip()
        selected_tests = selected_tests.replace("\n", " or ")
    else:
        selected_tests = ""

    return selected_tests


if __name__ == "__main__":
    selected_tests = get_selected_tests()

    # set the environment variable to be propagated to other steps
    print(f"##vso[task.setvariable variable=SELECTED_TESTS]'{selected_tests}'")

Review comment (suggestion):
I would use a similar logic as for the `get_commit_message.py`, something like this:

```py
if "BUILD_REASON" not in os.environ:
    raise RuntimeError("This legacy script is supposed to be used only on Azure. On GitHub Actions use the `SELECTED_TESTS` environment variable directly")
```

Code after:
import os

from get_commit_message import get_commit_message


def get_selected_tests():
    """Parse the commit message to check if pytest should run only specific tests.

    If so, selected tests will be run with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all".

    The commit message must take the form:
        <title> [all random seeds]
        <test_name_1>
        <test_name_2>
        ...
    """
    if "SELECTED_TESTS" in os.environ:
        raise RuntimeError(
            "This legacy script should only be used on Azure. "
            "On GitHub actions, use the 'SELECTED_TESTS' environment variable"
        )

    commit_message = get_commit_message()

    if "[all random seeds]" in commit_message:
        selected_tests = commit_message.split("[all random seeds]")[1].strip()
        selected_tests = selected_tests.replace("\n", " or ")
    else:
        selected_tests = ""

    return selected_tests


if __name__ == "__main__":
    # set the environment variable to be propagated to other steps
    selected_tests = get_selected_tests()

    if selected_tests:
        print(f"##vso[task.setvariable variable=SELECTED_TESTS]'{selected_tests}'")
        print(f"selected tests: {selected_tests}")  # helps debugging
    else:
        print("no selected tests")
Repository: scikit-learn/scikit-learn
File: build_tools/azure/get_selected_tests.py (Python)

Code before:
    """
    if "SELECTED_TESTS" in os.environ or os.environ.get("GITHUB_ACTIONS", False):
        raise RuntimeError(
            f"It seems that the `SELECTED_TESTS` environment variable is set or you are"
            f"in a GitHub Actions context "
            f"(GITHUB_ACTIONS={os.environ.get('GITHUB_ACTIONS', False)})."
            f"Instead, please use directly `SELECTED_TESTS`."
        )

    logger.debug("`selected_tests` coming from Azure")

    commit_message = get_commit_message()

    if "[all random seeds]" in commit_message:
        selected_tests = commit_message.split("[all random seeds]")[1].strip()
        selected_tests = selected_tests.replace("\n", " or ")
    else:
        selected_tests = ""

    return selected_tests


if __name__ == "__main__":
    selected_tests = get_selected_tests()

    # set the environment variable to be propagated to other steps
    print(f"##vso[task.setvariable variable=SELECTED_TESTS]'{selected_tests}'")
    print(f"selected tests: {selected_tests}")  # helps debugging


Review comment (suggestion):
Is there a good reason to change this? As a reviewer any changes that requires me having to think will make me unhappy 😅. Also since this is a legacy script, it will go away soonish and I would tweak it only when really needed.

Code after:
import os

from get_commit_message import get_commit_message


def get_selected_tests():
    """Parse the commit message to check if pytest should run only specific tests.

    If so, selected tests will be run with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all".

    The commit message must take the form:
        <title> [all random seeds]
        <test_name_1>
        <test_name_2>
        ...
    """
    if "SELECTED_TESTS" in os.environ:
        raise RuntimeError(
            "This legacy script should only be used on Azure. "
            "On GitHub actions, use the 'SELECTED_TESTS' environment variable"
        )

    commit_message = get_commit_message()

    if "[all random seeds]" in commit_message:
        selected_tests = commit_message.split("[all random seeds]")[1].strip()
        selected_tests = selected_tests.replace("\n", " or ")
    else:
        selected_tests = ""

    return selected_tests


if __name__ == "__main__":
    # set the environment variable to be propagated to other steps
    selected_tests = get_selected_tests()

    if selected_tests:
        print(f"##vso[task.setvariable variable=SELECTED_TESTS]'{selected_tests}'")
        print(f"selected tests: {selected_tests}")  # helps debugging
    else:
        print("no selected tests")
Repository: scikit-learn/scikit-learn
File: sklearn/utils/tests/test_tags.py (Python)

Code before:
)
from sklearn.utils.estimator_checks import (
    check_estimator_tags_renamed,
    check_valid_tag_types,
)


class EmptyClassifier(ClassifierMixin, BaseEstimator):
    pass


class EmptyTransformer(TransformerMixin, BaseEstimator):
    pass


class EmptyRegressor(RegressorMixin, BaseEstimator):
    pass


def test_type_error_is_thrown_for_class_vs_instance():
    """Test that a clearer error is raised if a class is passed instead of an instance.

    Related to the discussion in
    https://github.com/scikit-learn/scikit-learn/issues/32394#issuecomment-3375647854,
    this tests that a more descriptive error is raised if an estimator class is passed
    to `get_tags()` instead of an instance.
    """
    est = BaseEstimator
    with pytest.raises(TypeError, match="It seems that you passed an estimator class"):
        get_tags(est)


@pytest.mark.parametrize(
    "estimator, value",
    [
        [EmptyClassifier(), True],
        [EmptyTransformer(), False],
        [EmptyRegressor(), True],
        [BaseEstimator(), False],
    ],
)
def test_requires_y(estimator, value):
    assert get_tags(estimator).target_tags.required == value


def test_no___sklearn_tags__with_more_tags():
    """Test that calling `get_tags` on a class that defines `_more_tags` but not
    `__sklearn_tags__` raises an error.
    """

    class MoreTagsEstimator(BaseEstimator):

Review comment (refactor):
```suggestion
    https://github.com/scikit-learn/scikit-learn/issues/32394#issuecomment-3375647854.
```

Nit. I would remove this, since it repeats the information from the summary line.

Code after:
    get_tags,
)
from sklearn.utils.estimator_checks import (
    check_estimator_tags_renamed,
    check_valid_tag_types,
)


class EmptyClassifier(ClassifierMixin, BaseEstimator):
    pass


class EmptyTransformer(TransformerMixin, BaseEstimator):
    pass


class EmptyRegressor(RegressorMixin, BaseEstimator):
    pass


def test_type_error_is_thrown_for_class_vs_instance():
    """Test that a clearer error is raised if a class is passed instead of an instance.

    Related to the discussion in
    https://github.com/scikit-learn/scikit-learn/issues/32394#issuecomment-3375647854.
    """
    estimator_class = EmptyClassifier
    match = re.escape(
        "Expected an estimator instance (EmptyClassifier()), "
        "got estimator class instead (EmptyClassifier)."
    )
    with pytest.raises(TypeError, match=match):
        get_tags(estimator_class)


@pytest.mark.parametrize(
    "estimator, value",
    [
        [EmptyClassifier(), True],
        [EmptyTransformer(), False],
        [EmptyRegressor(), True],
        [BaseEstimator(), False],
    ],
)
def test_requires_y(estimator, value):
    assert get_tags(estimator).target_tags.required == value


def test_no___sklearn_tags__with_more_tags():
    """Test that calling `get_tags` on a class that defines `_more_tags` but not
    `__sklearn_tags__` raises an error.