FIX Target encoder const y #28233

s-banach · 2024-01-23T18:57:11Z

Reference Issues/PRs

Regarding #27879, test failure when pandas copy-on-write is enabled.

What does this implement/fix? Explain your changes.

The Cython code for TargetEncoder does not modify the target column y, so we mark it const.

Any other comments?

Simply adding const is now an option, since cython/cython#5230 is closed.

I assume someone else may want to improve the test here:

I didn't know how to modify the existing tests to fail, so I just made a new one.
I don't know what happens if I stick pd.options.mode.copy_on_write = True in one test.
Does this configuration carry forward to later tests in the test suite?

github-actions · 2024-01-23T18:58:35Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: abca02b. Link to the linter CI: here}

adrinjalali · 2024-01-25T11:35:52Z

CI fails to compile though. Do we need a newwer cython for this to compile?

As for setting the option on pandas, it should be in a context manager kind of thing, and adding a new test for this is okay.

s-banach · 2024-01-25T12:19:10Z

Evidently the version of cython on conda-forge is a couple weeks out of date. I know you guys need conda because your build system is so complicated, so I’ll just wait on this.

adrinjalali · 2024-01-25T12:37:33Z

@lesteve would know better where we need to change to upgrade the cython version on builds.

lesteve · 2024-01-25T12:57:07Z

If I understand correctly, it looks like you need Cython 3.0.8 to be our minimum Cython supported version, I think we need to discuss this outside of this PR. There was at least one other issue where Cython >= 3 was needed #27682.

I am fine with it personnally since numpy and scipy require Cython >=3 IIRC but feed-back needs to be gathered and then someone needs to update the lock-files. Also currently the lock-file update failed because pandas started to warn about PyArrow becoming a required dependency, so this needs to be handled too ...

I don't know what happens if I stick pd.options.mode.copy_on_write = True in one test. Does this configuration carry forward to later tests in the test suite?

Yes it does, so to avoid side-effects maybe pandas has a context manager or you need to reset it with a try/finally.

On It would be great to do a common test although it won't catch everything since it uses default parameter, see #27879 (comment)

adrinjalali · 2024-01-25T13:07:21Z

Opened #28258 to discuss pandas issue. I think we can simply update Cython, since the main issue was on cython==3.0, which is resolved now.

lesteve · 2024-01-25T13:16:37Z

I opened #28259 to get feed-back on Cython 3.0.8 being our minimum supported version. I am +1 for this.

adrinjalali · 2024-02-01T11:04:54Z

CI failing @s-banach

thomasjpfan

Thank you for the PR @s-banach !

thomasjpfan · 2024-02-02T00:58:33Z

sklearn/preprocessing/tests/test_target_encoder.py

@@ -701,3 +701,10 @@ def test_target_encoding_for_linear_regression(smooth, global_random_seed):
    # cardinality yet non-informative feature instead of the lower
    # cardinality yet informative feature:
    assert abs(coef[0]) < abs(coef[2])
+
+
+def test_27879():


Can we use a better function name?

Suggested change

def test_27879():

def test_pandas_copy_on_write_true():

"""Check target encoder works with copy on write.

Non-regression test for gh-27879.

"""

thomasjpfan · 2024-02-02T01:00:52Z

sklearn/preprocessing/tests/test_target_encoder.py

+
+
+def test_27879():
+    pd = pytest.importorskip("pandas")


For the failing CI, it is using a really old version of pandas that does not have mode.copy_on_write. Here we can set the min version here:

Suggested change

pd = pytest.importorskip("pandas")

pd = pytest.importorskip("pandas", minversion="2.0")

thomasjpfan · 2024-02-02T01:02:31Z

Please add an entry to the change log at doc/whats_new/v1.4.rst with tag |Fix|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

Sorry, I use all my brain power at work

adrinjalali

Otherwise LGTM.

doc/whats_new/v1.4.rst

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Ingkoun

Hhhjk

Co-authored-by: s-banach <john@hopfensperger.family> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

s-banach and others added 2 commits January 23, 2024 13:10

Quick fix.

7d9fafb

fix test

14f4255

github-actions bot added module:preprocessing cython labels Jan 23, 2024

s-banach marked this pull request as draft January 25, 2024 12:19

lesteve mentioned this pull request Jan 25, 2024

RFC bump Cython minimum supported version to 3.0.8 #28259

Closed

s-banach added 2 commits January 25, 2024 22:27

Change pandas config in context manager

31bfdc8

Merge branch 'main' into TargetEncoder-const-Y

8b91110

s-banach marked this pull request as ready for review January 31, 2024 18:50

Black format test (Whoops!)

056b767

thomasjpfan reviewed Feb 2, 2024

View reviewed changes

thomasjpfan changed the title ~~Target encoder const y~~ FIX Target encoder const y Feb 2, 2024

thomasjpfan added the To backport PR merged in master that need a backport to a release branch defined based on the milestone. label Feb 2, 2024

thomasjpfan added this to the 1.4.1 milestone Feb 2, 2024

s-banach added 4 commits February 1, 2024 22:11

Rename test, add pandas minversion to importorskip

ef73bd9

Update v1.4.rst

3dbc6af

fix: use suggested test name

82fa733

Waste more compute money for a spelling error

79ccd8f

Sorry, I use all my brain power at work

adrinjalali approved these changes Feb 2, 2024

View reviewed changes

doc/whats_new/v1.4.rst Outdated Show resolved Hide resolved

Update doc/whats_new/v1.4.rst

abca02b

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

thomasjpfan approved these changes Feb 2, 2024

View reviewed changes

thomasjpfan merged commit ce8cb6e into scikit-learn:main Feb 2, 2024
30 checks passed

Ingkoun approved these changes Feb 4, 2024

View reviewed changes

glemaitre pushed a commit that referenced this pull request Feb 13, 2024

FIX Target encoder const y (#28233)

b279e4e

Co-authored-by: s-banach <john@hopfensperger.family> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Target encoder const y #28233

FIX Target encoder const y #28233

-def test_27879():
+def test_pandas_copy_on_write_true():
+    """Check target encoder works with copy on write.
+    Non-regression test for gh-27879.
+    """

	pd = pytest.importorskip("pandas")
	pd = pytest.importorskip("pandas", minversion="2.0")

FIX Target encoder const y #28233

FIX Target encoder const y #28233

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

✔️ Linting Passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment