fix: Accept X and y as positional argument with as_dict=True in train_test_split #1570
Conversation
|
@auguste-probabl |
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
glemaitre
changed the title
glemaitre
changed the title
glemaitre
changed the title
| if y is not None: | ||
| new_arrays.append(y) | ||
| keys += ["y"] | ||
|
|
||
| if as_dict and arrays: |
There was a problem hiding this comment.
Why do you remove this? If both are true, it will cause a conflict.
|
|
||
| new_arrays = list(keyword_arrays.values()) | ||
|
|
||
| if X is not None: |
There was a problem hiding this comment.
Why is this repeated inside and outside the if? It is redundant.
| if X is not None: | ||
| new_arrays.append(X) | ||
| keys.append("X") | ||
| if y is not None: |
There was a problem hiding this comment.
Why is this repeated inside and outside the if? It is redundant.
| @@ -167,21 +177,20 @@ class labels. | |||
| stratify=stratify, | |||
| ) | |||
|
|
|||
| if X is None: | |||
| X = arrays[0] if len(arrays) == 1 else arrays[-2] | |||
| if X is None and len(arrays) >= 1: | |||
There was a problem hiding this comment.
Incorrect value for case when len(arrays)>1?
…thub.com/amltarek/skore into fix-Passing-all-datasets-by-keyword-1544
|
@auguste-probabl I have implemented the changes as you requested and also tested it. Additionally, I modified another function, test_train_test_split_dict_kwargs(), because it was throwing an error when data was passed without keyword arguments while return_dict=True. This issue has now been fixed. Furthermore, I addressed all the changes requested by @nkapila6. |
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Can you add a test with
arr1 = [[1]] * 20
arr2 = [0] * 10 + [1] * 10
train_test_split(arr2, z=arr1, as_dict=True)There was a problem hiding this comment.
this is tested through two functions named( test_train_test_split_check_dict()) and test_train_test_split_dict_kwargs().
There was a problem hiding this comment.
It's a bit different: right now we test either all arguments passed by keyword, or all arguments passed by position. I'd like to also test the combination of both (one array passed by position, one array passed by keyword).
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
skore/tests/unit/sklearn/train_test_split/test_train_test_split.py
Outdated
Show resolved
Hide resolved
|
I have removed all the duplicate functions. Can you check the code? @auguste-probabl |
| result = train_test_split( | ||
| X=X, | ||
| y=y, | ||
| sample_weights=weights, | ||
| test_size=0.2, | ||
| as_dict=True, | ||
| random_state=0, | ||
| ) |
There was a problem hiding this comment.
This test feels a bit redundant, but
train_test_split(
X,
y,
sample_weights=weights,
...
)
(i.e. a mix of positional and keyword arguments) would be interesting. See also my other comment
|
@amltarek Can you resolve comments when it's clear that you have addressed them? It helps review your code. |
|
Also please sign your commits |
Coverage Report for backend
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@auguste-probabl Can you check the updates? |
| def test_empty_input(): | ||
| """Tests that passing empty lists for X and y raises a ValueError.""" | ||
| X = [] | ||
| y = [] | ||
| with pytest.raises(ValueError): | ||
| train_test_split(X, y) |
There was a problem hiding this comment.
I don't think this test is needed; this behaviour is not specific to our function, but rather to sklearn's.
| assert "X_train" in result | ||
| assert "X_test" in result | ||
| assert "y_train" in result | ||
| assert "y_test" in result |
| >>> # When using positional arguments and as_dict=True | ||
| >>> # the first argument is assumed to be X, the second y | ||
| >>> train_test_split( | ||
| ... [[1], [2], [3], [4]], [0, 1, 0, 1], as_dict=True, random_state=0 |
There was a problem hiding this comment.
| ... [[1], [2], [3], [4]], [0, 1, 0, 1], as_dict=True, random_state=0 | |
| ... [[1], [2], [3], [4]], [0, 1, 0, 1], as_dict=True |
No need for random_state since we don't check the output arrays. You can also remove random_state in the previous doctest
There was a problem hiding this comment.
We should also test what happens with
train_test_split(X, X=X)I think there should be an error like
X cannot be passed both by position and by keyword.
Same for y.
closes #1544
I fixed the issue by updating the handling of keyword arguments when as_dict=True is used. Now, if all datasets are passed as keywords, the function directly returns them as a dictionary without extra processing. This makes the behavior more intuitive and avoids redundancy. I also tested the fix through test_train_split_test.py to ensure it works correctly.
