shuffle as boolean while converting dataset to dataclass

[mrastgoo]

closes #1178

adding shuffling as boolean in _fetch_dataset_as_dataclass and setting it to True in fetch_employee_salaries

[jeromedockes]

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?

[GaelVaroquaux]

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.

[mrastgoo]

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.

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 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.

[mrastgoo]

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 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 ?

[GaelVaroquaux]

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

[jeromedockes]

and IIUC the reordering is not optional and no new parameter is exposed to the user right?

[GaelVaroquaux]

and IIUC the reordering is not optional and no new parameter is exposed to the user right?

As you wish

[jeromedockes]

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

[Vincent-Maladiere]

Then what about editing the dataset, putting the reordered version on Figshare and fetching that directly?

[GaelVaroquaux]

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

[mrastgoo]

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

Hi @jeromedockes, Just for clarification, you prefer to modify the example code ? I can modify the PR according to that.

[jeromedockes]

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 datasets module.

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):

[mrastgoo]

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 datasets module.

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

[rcap107]

Due to various reworks in the code, and the fact this PR has not seen activity in a long time, it's better to just close it and start from scratch.