Conversation
|
Hi @solegalli - I'm sorry I'm just having git some trouble getting this branch into my cloned repo. I did the following: Am I doing this the right way? Sorry for the basic git question - thanks! |
bmreiniger
left a comment
bmreiniger
left a comment
There was a problem hiding this comment.
I was going to suggest using sklearn's check_X_y and/or check_array in a fashion similar to your usage of _check_y (could reduce some duplication in your check_X, and uses a public sklearn method instead of a private one), but there's a lot going on there for converting to array/sparse that would be a waste. Hrm...
Otherwise, minor comments.
| def _check_X_y(X, y): | ||
| X = check_X(X) | ||
| y = check_y(y, multi_output=multi_output, y_numeric=y_numeric) | ||
| check_consistent_length(X, y) | ||
| return X, y |
There was a problem hiding this comment.
In every case below you call this function, so consider just running this code first without defining it as a local function.
There was a problem hiding this comment.
Yes, makes sense. The thing is, if I ran this function first, then X and y would be a pandas df and series, so I can't really check for the different scenarios any more. That's why I took this convoluted approach instead.
How would you approach it?
There was a problem hiding this comment.
Oh, of course. This is fine then. You could also maybe define "the" index in cases, then do the checks/conversions?
| Parameters | ||
| ---------- | ||
| X: Pandas DataFrame or numpy ndarray | ||
| y: Pandas Series or numpy ndarray |
| validated as a vector. y cannot have np.nan or np.inf values if | ||
| multi_output=True. | ||
|
|
||
| y_numeric : bool, default=False |
There was a problem hiding this comment.
default in docstring disagrees with code default
| raise ValueError("y cannot be None.") | ||
|
|
||
| elif isinstance(y, pd.Series): | ||
| _check_y(y, multi_output=multi_output, y_numeric=y_numeric) |
There was a problem hiding this comment.
by not assigning, you get all the checks without the conversion, but e.g. the numeric conversion also doesn't happen...
There was a problem hiding this comment.
Good catch, thank you! Refactoring.
You need to push the branch to your own repo, and make a PR from there to the main repo's checkky branch. |
solegalli
changed the title
|
Thank you @bmreiniger @noahjgreen295 for raising and your support on this issue. Merging. |
3 participants
closes #377
closes #376
closes #365
@noahjgreen295 @bmreiniger
I am bringing the changes from #377 to this new PR at the back of former PR #408
Regarding the X and y checks, I am bringing in sklearn functionality to check y and then to check X and y (taken from their check_X_y function). I think that decreases the amount of new things we need to code.
I think I captured most of what we discussed in #377 , and I brought forward the tests and ideas from @noahjgreen295 as they were or slightly reformatted and also the suggestions from @bmreiniger.
Would you guys have a look and let me know what you think?
Let me know if you are OK to merge.
Thank you so much for raising and also for the support on this issue.