fix: improve the error message provided by deduplicate

[MarieSacksick]

Bug Fix Pull Request

Description

Addresses #673

Checklist

• I have read the contributing guidelines
• I have added tests that verify the bug fix
• I have added an entry to CHANGES.rst describing the fix
• My code follows the code style of this project
• I have checked my code and corrected any misspellings

How Has This Been Tested?

AI Disclosure

• This PR contains AI-generated code
  • I have tested the code generated in my PR
  • I have read and understood every line that has been generated by the AI agent
  • I can explain what the AI-generated code does

[rcap107]

Thanks for the PR @MarieSacksick!

[rcap107]

I'm wondering if the warning is needed, we almost never use warnings in skrub 🤔

[MarieSacksick]

True that it's a bit unusual for skrub. But it seems to me that it would be nice to warn the user that no deduplication was done. And I don't want to add a systematic warning that will be ignored and will push the user to remove all warnings...

[jeromedockes]

i also wouldn't add the warn parameter. I would say either we consider that there are no duplicates to find and return the original list without warnings, or we consider it is a genuine failure and raise a (informative) exception

[MarieSacksick]

This situation might happen regularly, and I don't want to break pipelines while deduplicate could be used to clean data "in case". I'll remove the warn parameter, but add this behavior in documentation.

[rcap107]

I'm fine with that solution 👍

[MarieSacksick]

Now that I think of it, why is it a function and not a transformer? Is there a way for the user to know the clusters or not?
It looks like I can only deduplicate a whole dataset, and not fit on train and apply on a predict.

[jeromedockes]

Now that I think of it, why is it a function and not a transformer? Is there a way for the user to know the clusters or not? It looks like I can only deduplicate a whole dataset, and not fit on train and apply on a predict.

You are completely right but I would consider it as low-priority because AFAICT deduplicate mostly works on synthetic examples crafted for deduplicate. On the 2-3 real use cases I tried I could not get anything useful out of it.

[rcap107]

Now that I think of it, why is it a function and not a transformer? Is there a way for the user to know the clusters or not? It looks like I can only deduplicate a whole dataset, and not fit on train and apply on a predict.
You are completely right but I would consider it as low-priority because AFAICT deduplicate mostly works on synthetic examples crafted for deduplicate. On the 2-3 real use cases I tried I could not get anything useful out of it.

I second this. I don't trust deduplicate either, and I'd prefer if the function were not used or publicized as much as it is.
If anything, I'd rather make the shortcomings clearer, because I feel like users are getting an overly optimistic view of the capabilities of this feature.

As for the historical reasons, this was added as a function in #339. The discussion includes more context on why it was a function rather than a class.