-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shuffle as boolean while converting dataset to dataclass #1213
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mrastgoo, thank you for this PR!
Your tests fail because of network issues with OpenML. This is unrelated to your PR, so I reran them.
@@ -702,6 +703,8 @@ def _fetch_dataset_as_dataclass( | |||
df = pd.read_parquet(info["path"]) | |||
else: | |||
df = pd.read_csv(info["path"], **read_csv_kwargs) | |||
if shuffling: | |||
df = shuffle(df, random_state=42).reset_index(drop=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the random state should be a function parameter, as it would allow for control for multiple seeds during testing and more fine-grained control on reproducibility.
I'm not 100% sure we should reset indices, because it could be surprising for users. WDYT?
Thanks @mrastgoo! I'm not sure we need to add a parameter to the fetcher, at least it is not required for addressing #1178 . we could shuffle it in the the example code (which by default is folded for the tabular_learner example, and which is not shown for the tablereport) rather than in the fetcher itself. WDYT? |
I'm not sure we need to add a parameter to the fetcher, at least it is not required for addressing [1]#1178 . we could shuffle it in the the example code (which by default is folded for the tabular_learner example, and which is not shown for the tablereport) rather than in the fetcher itself
I'd rather keep the code examples as simple as possible (every character counts), and shuffle in the fetcher, by default, in a reproducible way. And for this, an easy way of doing things would be to cut and reorder the dataset at a fix index, like "cutting" a deck of cards.
|
I can see why you prefer that, in this way the fetcher will return the original data as it is, with the same index. However it will make the example longer. |
I am not sure to understand the purpose of "cutting" @GaelVaroquaux, how many cuts do we do ? Do we want to shuffle as well ? and should we keep the orginal index or reorder them ? |
I am not sure to understand the purpose of "cutting" @GaelVaroquaux,
The whole goal of the modification is to have the first few lines not as nasty.
how many cuts do we do ?
Let's try one.
Do we want to shuffle as well ?
No, that way it's stable and reproducible (random number generators are not always reproducible across hardware)
and should we keep the orginal index or reorder them ?
Not reorder, but reset to avoid something looking strange
|
and IIUC the reordering is not optional and no new parameter is exposed to the user right? |
and IIUC the reordering is not optional and no new parameter is exposed to the user right?
As you wish
|
> and IIUC the reordering is not optional and no new parameter is exposed to the user right?
As you wish
Ok in that case I'd rather not add a parameter, just do the transformation every time
|
Then what about editing the dataset, putting the reordered version on Figshare and fetching that directly? |
Then what about editing the dataset, putting the reordered version on Figshare and fetching that directly?
I always like having a form of traceability of the data, so I think that I would prefer not changing the upstream data
|
Hi @jeromedockes, Just for clarification, you prefer to modify the example code ? I can modify the PR according to that. |
Hey @mrastgoo ! really sorry about the delay getting back to you. The reason is that there has been a major revamp of the dataset fetchers after we ran into some problems with how they were downloaded before and to simplify and improve the What I suggest is that we keep the plan of not modifying the downloaded file for now, but as discussed change the order of rows after loading the dataframe. The function you need to modify is now here: https://github.com/skrub-data/skrub/blob/main/skrub/datasets/_fetching.py#L32 (There have been so many changes that it might be easier to start a new PR to avoid git conflicts, sorry about that) the new function might look something like diff --git a/skrub/datasets/_fetching.py b/skrub/datasets/_fetching.py
index 0e6a5724..a2537776 100644
--- a/skrub/datasets/_fetching.py
+++ b/skrub/datasets/_fetching.py
@@ -29,7 +29,13 @@ def fetch_employee_salaries(data_home=None):
- y : pd.DataFrame, target labels
- metadata : a dictionary containing the name, description, source and target
"""
- return load_simple_dataset("employee_salaries", data_home)
+ data = load_simple_dataset("employee_salaries", data_home)
+ df = data['employee_salaries']
+ new_df = ... # re-order the dataframe
+ data['employee_salaries'] = new_df
+ data['X'] = new_df.drop(columns='current_annual_salary')
+ data['y'] = new_df['current_annual_salary']
+ return data
def fetch_medical_charge(data_home=None): |
Thanks @jeromedockes for the feedback, I try to finish the PR this weekend |
closes #1178
adding
shuffling
as boolean in_fetch_dataset_as_dataclass
and setting it to True infetch_employee_salaries